Repository navigation
Phase 1: React editor foundation (store, REST, blocks, shell) - #291
Merged
Merged
Conversation
Stand up a production-ready Vite pipeline for the new visual editor package alongside the existing Phase 0 spike build. Adds a placeholder entry point, a dedicated editor Vite config that externalizes React/ReactDOM/JSX runtime, a dev HTML mount, and new npm scripts (`dev:editor`, `build:editor`). The spike build at public/editor-spike/ is untouched. - New entry: resources/js/visual-editor/editor/main.tsx (placeholder) - New dev HTML: resources/js/visual-editor/editor/index.html - New config: vite.editor.config.ts (root switches per command, builds to dist/editor/) - Externalize react, react/jsx-runtime, react/jsx-dev-runtime, react-dom, react-dom/client - tsconfig.json: add @editor/* path alias + visual-editor include - package.json: add build:editor and dev:editor scripts - .gitignore: ignore dist/ and .vite/ cache directories Closes #263
…n-vite-config-react-externalized feat: Phase 1.1 — production Vite config with dev server + externalized React
Adds the Phase 1.2 entry points so host apps can render the React editor: - resources/views/editor/mount.blade.php renders a #ve-root element carrying data-post-id, data-post-type and data-api-base, and pulls the editor entry through @viteReactRefresh + @Vite. - resources/js/visual-editor/editor/main.tsx reads and types those data attributes, mounts an EditorShell placeholder, and logs clear console errors when the mount point or required attributes are missing instead of failing silently. - resources/js/visual-editor/editor/index.html is updated so the standalone Vite dev spike still boots against the new #ve-root contract. - routes/web.php registers a new /editor route alongside the existing /editor-spike route, which stays in place until the Phase 1 cleanup. - A Feature test locks in the view, route, and boot script contract. Closes #264
readEditorConfig previously accepted whitespace-only values like " " because the raw dataset strings were truthy. Trim each value before the guard so malformed attributes log the clear console.error instead of mounting with garbage config. Refs #264
…nt-view-react-editor-boot feat: blade mount view and boot script for React editor
Add the Phase 1 state foundation that replaces the Phase 0 spike's shared-mutable-state + __bumpTemplate pattern. createEditorStore() returns a plain zustand store (no middleware) owning the block tree, selection, and dirty flag, with immutable insert/update/remove/move, replaceBlocks, select/clearSelection, and markDirty/markClean actions. Selectors (useBlock, useChildren, useSelection, useIsDirty) are memoized React hooks. Vitest coverage spans every action and selector including nested trees, edge cases, and round-trip mutations. Closes #265
…remove Two CodeRabbit follow-ups on the Zustand editor store: 1. insertIntoTree used blocks.map(...) which always returned a new array, so insertBlock with an unknown parentClientId produced a fresh blocks reference and flipped isDirty. It now tracks a changed flag and returns the original blocks when nothing matched, making insertBlock a true no-op for stale parent ids. 2. removeBlock only cleared the selection when the removed clientId exactly matched the current selection. Removing an ancestor containing the selected block left a dangling clientId pointing at nothing. It now checks whether the selected block still exists in nextBlocks and resets selection to the initial value when it does not.
…r-store feat: Zustand editor store — block tree, selection, dirty flag (#265)
Add a hand-rolled history middleware wrapping the Zustand editor store. Content-affecting actions (insertBlock, updateBlockAttributes, removeBlock, moveBlock, replaceBlocks) push the previous blocks snapshot onto a bounded 100-entry past stack; selection and dirty-flag actions do not. Rapid updateBlockAttributes calls targeting the same block within a 500ms window coalesce into a single history entry, so undoing a typing burst rolls back the whole burst at once. Adds undo() / redo() actions, useCanUndo / useCanRedo selectors, and 19 Vitest cases covering round-trips, bottom/top no-ops, selection isolation, coalescing (same block, cross-block, window expiry, non-update barrier), bounded stack eviction, and action no-ops. Closes #266
Time-travel could previously leave the store at a blocks snapshot that diverges from the last markClean checkpoint while still reporting isDirty=false, because undo/redo spread the prior state verbatim. Set isDirty=true in both handlers so navigating the history is always treated as a modification relative to the last saved baseline.
…tory-store feat: undo/redo history layer on editor store
…tension-audit-paragraph-node-strategy docs: Tiptap extension audit and paragraph node strategy (Phase 1)
Validate that @dnd-kit/core + @dnd-kit/sortable coexist with @tiptap/react before #271 integrates dnd-kit into production BlockList. The harness lives under resources/js/visual-editor/editor/__dnd-kit-spike__/, reuses the same useTiptap hook production blocks use, and is excluded from the editor bundle via rollupOptions.input (dist/editor/main.js stays 1.20 kB). All four interaction checks from PHASE-0-FINDINGS.md finding #3 pass: drag handles do not interfere with Tiptap text selection, pointer sensor uses an 8px distance threshold to avoid contenteditable conflicts, stable id-keyed reorders preserve Tiptap state in remaining blocks, and the keyboard sensor is scoped to the handle so Tiptap keyboard handling is never hijacked. Findings and carry-forward constraints for #271 are documented in docs/phase-1-dnd-kit-findings.md. Closes #268
SpikeBlock.tsx imports CSS from @dnd-kit/utilities, which was previously only available transitively through @dnd-kit/sortable. Pin it explicitly so the import resolves independently of sortable's install tree.
…-harness feat: dnd-kit + Tiptap spike harness (#268)
Port BlockContextProvider, useInnerBlocksProps, and useBlockPreview from the Phase 0 spike into resources/js/visual-editor/editor/primitives/ with strict types and Zustand store integration. - BlockContext: typed selector hook with runtime type guard (no `as T` escape hatches) and stable refs across renders - EditorStoreContext: new React context exposing the editor store to primitives - useInnerBlocksProps: reads children from the Zustand store, exposes appendBlock/removeBlock/moveBlock APIs, and propagates clicks to store selection via capture - useBlockPreview: renders inert, aria-hidden, pointer-events:none previews backed by an ephemeral store - Block registry ported to the production tree - Vitest coverage for each primitive (happy path + edge cases) Spike copies under resources/js/editor-spike/primitives/ are left in place per PHASE-0-FINDINGS.md; cleanup happens in the final Phase 1 issue. Closes #269 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ensures the spy is restored even if the toThrow assertion fails, preventing spy leakage between tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-spike-primitives feat: productionize spike primitives with store integration
Build the React component skeleton that frames the editor: EditorShell, Canvas, BlockList, and BlockWrapper. No block-type-specific code — just the layout, the canvas that holds the tree, the block list that iterates the store, and the block wrapper that handles selection, hover, and a drag-handle slot for every block type. - EditorShell wraps the tree in EditorStoreProvider, installs global keyboard bindings (Cmd/Ctrl+Z undo, Cmd/Ctrl+Shift+Z and Ctrl+Y redo), and renders a dirty/selection statusbar - Canvas provides the editable chrome region - BlockList uses the productionized useInnerBlocksProps from #269 and maps top-level blocks to BlockWrappers - BlockWrapper adds hover/selection outlines, a drag-handle slot for #271, and delegates rendering to RenderBlock from the primitives - Minimal editor.css for block boundaries, selection/hover states, and drag-handle affordance - main.tsx now creates a real editor store seeded with two placeholder blocks and renders <EditorShell store={store} /> - Vitest coverage for BlockWrapper, BlockList, and EditorShell (16 new tests) — selection flows, keyboard undo/redo, hover/select classes, statusbar state Closes #270 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…canvas-blocklist-blockwrapper feat: editor shell — EditorShell, Canvas, BlockList, BlockWrapper
Wraps BlockList in DndContext + SortableContext with pointer (distance: 8) and keyboard sensors, turns the BlockWrapper drag-handle slot into a real accessible button via setActivatorNodeRef, and dispatches moveBlock on drop for top-level reorders only. Adds a DragOverlay, screen-reader announcements, and instructions covering the full keyboard drag flow (space to pick up, arrows to move, space to drop, escape to cancel). Tests cover wiring (handles, sensors, click-through, a11y) and drag dispatch by mocking DndContext to drive onDragEnd for pointer and keyboard paths — jsdom lacks layout, so the sensors cannot compute drop targets through it. Closes #271 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…list-integration feat: integrate dnd-kit into production BlockList (top-level)
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
`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.
…ding-rich-text-toolbar feat: paragraph/heading blocks with shared rich-text toolbar
Adds the first two ways to insert new blocks into the canvas: a sidebar panel with a searchable block list, and an in-editor slash command that replaces the current paragraph with a chosen block type. - New `editor/inserter/` module: cached-snapshot registry, built-in seeds for paragraph/heading, block factories, API loader with silent fallback (for #274), `insertBlockAtSelection`, and an atomic `replaceBlockWithInserterBlock` that commits replacement in a single undo step. - `InserterPanel` renders a searchable, click-to-insert list wired into the editor shell next to the canvas. - `SlashCommandPopover` renders the slash menu; `ParagraphEdit` detects `/query` in the block's plain text, filters the list, handles ArrowUp/ArrowDown/Escape via a window listener, routes Enter through Tiptap's keymap to commit the replacement, and suppresses the backspace-merge path while the slash popover is open. - History snapshots now capture `{ blocks, selection }` so undo/redo restore the caret to the block that was being edited at the time of the change. This fixes the focus-loss after undoing a slash command replacement — without it, the replaced block's clientId was gone and nothing was selected, leaving the editor unfocused. - Boot sequence calls `loadInserterBlocks({ apiBase })`. Closes #273
- BlockTreeRule: enforce clientId uniqueness and bound recursion via MAX_DEPTH (10) and MAX_NODES (1000) so untrusted payloads cannot blow the stack or burn unbounded CPU. - BlockTypeRegistry::register(): trim + validate names against a namespaced lowercase-kebab pattern and throw InvalidArgumentException on empty or malformed input. - VisualEditorPost: switch from the Laravel 11+ casts() method to the \$casts property so the package stays compatible with the declared illuminate/support >=5.3 constraint. - docs/phase-1-rest-api.md: clarify that the api + auth middleware stack is the default and overridable via config. - Add Pest coverage for duplicate clientIds, depth limit, node limit, and BlockTypeRegistry name validation.
Mirrors the existing GET coverage so both read and write paths exercise the policy rejection when artisanpack.visual-editor.authorization.restrict_by_owner is enabled. Also asserts the original blocks are unchanged to confirm the Gate check fires before the controller persists anything.
…s-blocks feat: REST API for posts GET/PUT and blocks GET (Phase 1.12)
Boots the React editor from the package's REST endpoints so opening
/editor/{id} loads the post via GET, edits debounce a PUT autosave,
and reloads pick up persisted changes. Adds a direct Zustand store
subscription in useAutosave so debounce/save cycles survive every
isDirty transition, not just the first one.
Includes Vitest coverage for the REST client, autosave happy/error/
retry paths, and the EditorBoot load + retry flow.
Closes #275
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s idle When saveStatus.status === 'idle', the label falls back to the dirty/saved text but statusToken was still reporting 'idle', so data-ve-save-status could diverge from what the user sees. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…or-rest-load-save-persistence feat: wire editor to REST load, autosave, reload persistence
Phase 1.14 — completes the React migration's Phase 1 goal. After this lands, the visual editor tree contains no Alpine-era or spike-era scaffolding. - Remove resources/js/editor-spike/ (Phase 0 architecture spike) - Remove resources/js/visual-editor/editor/__dnd-kit-spike__/ (Phase 1 dnd-kit harness that depended on @spike/richtext/useTiptap) - Remove editor-spike route, blade view, index.html entry, and the visual-editor-spike-assets publish tag - Consolidate vite.editor.config.ts into the root vite.config.ts and drop the spike-only vite.config.ts; simplify npm scripts - Drop the @spike path alias from tsconfig.json and vitest.config.ts - Move the Tiptap/jsdom test setup into resources/js/visual-editor/editor/test-setup.ts - Update EditorMountViewTest route assertion and remove public/editor-spike/ from .gitignore - Mark Phase 1 as landed in REACT-MIGRATION.md Closes #276 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…pine-editor-phase-0-spike chore: delete Alpine editor remnants and Phase 0 spike
- BlockTypeRegistry::unregister() now trims the name so entries registered with surrounding whitespace can be removed correctly - EditorBoot accepts the full props object so the postType prop required by callers (main.tsx, tests) passes type-checking under noUnusedParameters while remaining reserved for block-context wiring Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPhase‑0 editor‑spike artifacts were removed and replaced by a Phase‑1 visual editor: a React + TipTap frontend with a Zustand store, dnd‑kit reordering, inserter UI, autosave and REST endpoints, server-side models/controllers/validation/policies, migrations, and many tests and build-tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Boot as EditorBoot
participant Store as EditorStore
participant Autosave as useAutosave
participant REST as REST API
participant Controller as PostsController
participant DB as Database
Browser->>Boot: load (reads data-* from mount)
Boot->>REST: GET /visual-editor/api/posts/{post}
REST->>Controller: VisualEditorPostsController::show
Controller->>DB: query ve_contents
DB-->>Controller: post payload
Controller-->>Boot: JSON payload
Boot->>Store: createEditorStore(payload.blocks)
Note over Browser,Store: User edits content
Browser->>Store: mutate blocks (insert/update)
Store-->>Autosave: isDirty change triggers debounce
Autosave->>REST: PUT /visual-editor/api/posts/{post} (debounced/with CSRF)
REST->>Controller: VisualEditorPostsController::update
Controller->>DB: persist blocks
DB-->>Controller: saved record
Controller-->>Autosave: response (saved)
Autosave-->>Store: markClean (if snapshot matches)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (18)
resources/js/visual-editor/editor/components/Icon.tsx (1)
11-19: Treat whitespace-only titles as missing.A title like
" "currently setsrole="img"without a meaningful label. Trim first to avoid accidental accessibility noise.Suggested refactor
export function Icon({ icon, className, title }: IconProps) { - const hasTitle = typeof title === 'string' && title.length > 0; + const normalizedTitle = typeof title === 'string' ? title.trim() : ''; + const hasTitle = normalizedTitle.length > 0; return ( <FontAwesomeIcon icon={icon} className={className} - title={title} + title={hasTitle ? normalizedTitle : 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 11 - 19, The component currently treats whitespace-only strings as valid titles; update the title handling by trimming before checking length: change hasTitle to use title.trim() (e.g., hasTitle = typeof title === 'string' && title.trim().length > 0), pass the trimmed title to FontAwesomeIcon when hasTitle is true (title={hasTitle ? title.trim() : undefined}), and keep role and aria-hidden logic based on this new hasTitle so whitespace-only titles are treated as missing.database/migrations/2026_04_14_000000_create_ve_contents_table.php (1)
29-29: Add foreign key constraint toauthor_idto prevent orphaned ownership records.Since
author_idreferences the users table (based on policy authorization logic), define a foreign key withnullOnDelete()to maintain referential integrity while preserving the nullable design:$table->foreignId( 'author_id' )->nullable()->constrained( 'users' )->nullOnDelete();This prevents scenarios where a user is deleted but orphaned
ve_contentsrecords remain, which could affect access control audits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_04_14_000000_create_ve_contents_table.php` at line 29, Replace the nullable unsignedBigInteger column definition for author_id in the CreateVeContentsTable migration with a nullable foreign key that references users and uses nullOnDelete; specifically update the schema change around the author_id definition in the CreateVeContentsTable migration (the ve_contents table setup) to use a foreignId constrained to 'users' with nullOnDelete so deleted users set author_id to null and prevent orphaned ownership records.routes/web.php (1)
7-13: Hardcoded route parameters limit real-world usage.The route hardcodes
postId => '1'andpostType => 'page', which is fine for Phase 1 scaffolding/demo purposes but won't support editing arbitrary posts. Consider parameterizing this route or documenting that it's a temporary dev entry point.-Route::get('/editor', function () { +Route::get('/editor/{postType}/{postId}', function (string $postType, string $postId) { return view('visual-editor::editor.mount', [ - 'postId' => '1', - 'postType' => 'page', + 'postId' => $postId, + 'postType' => $postType, 'apiBase' => '/api', ]); })->name('visual-editor.editor');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/web.php` around lines 7 - 13, The route closure for Route::get('/editor', ...)->name('visual-editor.editor') currently passes hardcoded view params 'postId' => '1' and 'postType' => 'page'; change it to accept dynamic inputs (e.g., route parameters like {post} and {type} or read from query string) and forward those values into the view 'visual-editor::editor.mount' instead of the hardcoded literals, validating/casting the inputs as needed (or mark the route clearly as a temporary dev-only entry in comments/config if you intend to keep hardcoded defaults). Ensure you update any callers/links to use the new parameters and keep the route name 'visual-editor.editor' unchanged.resources/js/visual-editor/editor/rest/__tests__/postRestClient.test.ts (1)
93-132: Add CSRF-header parity test forsavePost.
fetchPostvalidates CSRF header behavior, butsavePostcurrently doesn’t. Adding that assertion would close a small regression gap.✅ Suggested test addition
describe('savePost', () => { @@ it('throws PostRestError on validation failure', async () => { @@ }); + + it('attaches a CSRF token when provided', async () => { + const fetchImpl = vi.fn().mockResolvedValue(mockJsonResponse(samplePayload)); + + await savePost('1', blocks, { + apiBase: '/visual-editor/api', + fetchImpl, + csrfToken: 'token-xyz', + }); + + const headers = fetchImpl.mock.calls[0][1].headers as Headers; + expect(headers.get('X-CSRF-TOKEN')).toBe('token-xyz'); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/rest/__tests__/postRestClient.test.ts` around lines 93 - 132, The savePost tests are missing a CSRF-header assertion; update the 'PUTs the block tree and returns the persisted payload' spec for savePost to also assert the fetch call includes the same CSRF header checked in fetchPost (e.g. ensure (init.headers as Headers).get('X-WP-Nonce') (or the project’s CSRF header name) is present and equals the expected test nonce value provided by the test setup), locating the call via the savePost test's fetchImpl.mock.calls and adding the header assertion alongside the existing method/content-type/body checks.tests/Feature/EditorMountViewTest.php (1)
14-15: Add explicit file-read assertions for clearer failures.If
file_get_contentsreturnsfalse, current failures become less actionable than they need to be.♻️ Proposed refactor
$viewPath = __DIR__.'/../../resources/views/editor/mount.blade.php'; $routesPath = __DIR__.'/../../routes/web.php'; $bootPath = __DIR__.'/../../resources/js/visual-editor/editor/main.tsx'; + +function mustReadFile(string $path): string { + $contents = file_get_contents($path); + expect($contents)->not->toBeFalse(); + return $contents; +} @@ test('mount blade view renders the ve-root element with data attributes', function () use ($viewPath) { - $contents = file_get_contents($viewPath); + $contents = mustReadFile($viewPath); @@ test('mount blade view pulls the editor entry through `@vite`', function () use ($viewPath) { - $contents = file_get_contents($viewPath); + $contents = mustReadFile($viewPath); @@ test('package routes register the /editor path', function () use ($routesPath) { - $contents = file_get_contents($routesPath); + $contents = mustReadFile($routesPath); @@ test('boot script targets `#ve-root` and logs a clear error when missing', function () use ($bootPath) { - $contents = file_get_contents($bootPath); + $contents = mustReadFile($bootPath);Also applies to: 23-24, 29-30, 40-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/EditorMountViewTest.php` around lines 14 - 15, The test reads view files using file_get_contents into $contents but doesn't assert the read succeeded, so failures are unclear when file_get_contents returns false; update EditorMountViewTest to assert the read before further assertions (e.g. use $this->assertNotFalse($contents) or $this->assertIsString($contents) and optionally $this->assertNotEmpty($contents)) immediately after each file_get_contents call (references: variable $contents and the file_get_contents(...) calls present in the test at the multiple spots noted) so subsequent assertions operate on a valid string.resources/js/visual-editor/editor/rest/__tests__/useAutosave.test.tsx (1)
51-53: MoveonStateout of render.Calling
onState(state)during render makes the harness impure and will become noisy fast if a future caller updates React state there. Push the notification into an effect instead.♻️ Proposed refactor
+import { useEffect } from 'react'; + function AutosaveHarness({ store, fetchImpl, onState, @@ const state = useAutosave({ store, postId: '1', clientOptions: { apiBase: '/visual-editor/api', fetchImpl }, debounceMs, retryBaseMs, maxRetries, }); - if (onState) { - onState(state); - } + useEffect(() => { + onState?.(state); + }, [onState, state]); return <div data-testid="status">{state.status}</div>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/rest/__tests__/useAutosave.test.tsx` around lines 51 - 53, The test currently calls onState(state) during render (inside the render path where onState is checked), which makes the harness impure; change this by removing the direct call from render and invoking onState from a React effect instead: create a useEffect (or the test harness equivalent) that runs when state (and onState) change, and inside it guard if (onState) onState(state); include onState and state in the effect dependency array so notifications occur on updates and avoid invoking callbacks during render.src/Policies/VisualEditorPostPolicy.php (1)
84-91: Method nameisOwnerOrAdminis misleading.The method only checks ownership, not admin status. Consider renaming to
isOwnerto accurately reflect its behavior, or add actual admin role checking if that's intended.♻️ Proposed rename for clarity
- protected function isOwnerOrAdmin( Authenticatable $user, VisualEditorPost $post ): bool + protected function isOwner( Authenticatable $user, VisualEditorPost $post ): bool { if ( null === $post->author_id ) { return true; } return (int) $post->author_id === (int) $user->getAuthIdentifier(); }Also update the calls on lines 42 and 60:
- return $this->isOwnerOrAdmin( $user, $post ); + return $this->isOwner( $user, $post );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Policies/VisualEditorPostPolicy.php` around lines 84 - 91, The method VisualEditorPostPolicy::isOwnerOrAdmin only checks ownership, so rename it to isOwner (or implement real admin-checking if admin behavior is desired); update all call sites that invoke isOwnerOrAdmin (e.g., the two places that currently call it in this policy) to call isOwner instead, and update any PHPDoc/type hints and tests referencing isOwnerOrAdmin to use the new name so behavior and naming stay consistent.resources/js/visual-editor/editor/components/SlashCommandPopover.tsx (1)
16-22: Consider adding ARIA listbox semantics for better screen reader support.The popover has an
aria-label, but addingrole="listbox"on the<ul>androle="option"on list items witharia-selectedwould improve screen reader navigation. This can be deferred to the accessibility sweep mentioned in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/components/SlashCommandPopover.tsx` around lines 16 - 22, The popover's markup in SlashCommandPopover lacks ARIA listbox semantics; update the rendered list container (the <ul> in the component) to include role="listbox" and each list item (rendered options in the component's map) to include role="option" and an appropriate aria-selected attribute reflecting selection state (true/false); ensure you update the render logic in the SlashCommandPopover component where the option elements are created so screen readers can navigate the list properly.resources/js/visual-editor/editor/blocks/paragraph/edit.tsx (1)
111-138: Reference toeditorbefore assignment in callbacks.The
onEnterandonBackspaceAtStartcallbacks referenceeditor(lines 124, 133) buteditoris assigned from the return value ofuseBlockTiptap. Since hooks return synchronously and these callbacks are only invoked after the editor is mounted, this works in practice. However, the TypeScript checker may flag this as a potential issue, and the pattern is subtle.Consider restructuring to make the dependency clearer, or add a comment explaining why this is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/blocks/paragraph/edit.tsx` around lines 111 - 138, The callbacks passed into useBlockTiptap (onEnter, onBackspaceAtStart) reference the editor variable returned from useBlockTiptap which can look like a forward reference to TypeScript; either capture the editor in a stable ref and use that ref.current inside onEnter/onBackspaceAtStart, or rewrite the callbacks to receive the editor instance (or a guaranteed non-null typed editor) before calling handleBlockEnter/handleBlockBackspace; update references to useBlockTiptap, onEnter, onBackspaceAtStart, editor, handleBlockEnter, handleBlockBackspace, slashActiveRef, selectSlashBlockAtIndex, filteredRef, slashIndexRef, store and clientId accordingly, or alternatively add a concise inline comment explaining why the current pattern is safe and assert the editor type to satisfy TypeScript.resources/js/visual-editor/editor/components/EditorShell.tsx (1)
140-153: Clarify the comment about select elements.The comment suggests
<select>elements should keep native behavior, but they don't have native undo/redo—so the currentreturn falseis correct. Consider rewording to avoid confusion.📝 Suggested comment clarification
- // Non-contenteditable focusable fields (e.g. `<select>`) should also keep - // their native behavior. The Tiptap editor surfaces are contenteditable, - // so those still route through the store's undo/redo. - return false; + // Contenteditable elements (TipTap) and other focusable fields like + // <select> don't have native undo/redo, so they route through the + // store's history. + return 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/EditorShell.tsx` around lines 140 - 153, The comment in shouldDeferToNativeHistory is misleading about <select> elements implying they have native undo/redo; update the explanatory comment around shouldDeferToNativeHistory (and the sentence mentioning `<select>`) to state that non-contenteditable focusable controls (e.g., <select>) should preserve their native keyboard behavior even though they do not provide native undo/redo, and clarify that only contenteditable Tiptap surfaces route undo/redo through the store; leave the logic (return false for other elements) unchanged.resources/js/visual-editor/editor/inserter/__tests__/inserterRegistry.test.ts (1)
181-190: Consider strengthening the assertion for "ignores non-ok responses".The test verifies
fetchImplwas called but doesn't assert the expected outcome (built-ins remain registered, no API blocks added). Adding assertions similar to the "falls back to built-ins" test would make this more robust.💡 Suggested improvement
it('ignores non-ok responses', async () => { const fetchImpl = vi.fn().mockResolvedValue({ ok: false, json: async () => ({ blocks: [] }), }) as unknown as typeof fetch; await loadInserterBlocks({ apiBase: 'https://example.test/api', fetchImpl }); expect(fetchImpl).toHaveBeenCalledOnce(); + const names = getInserterBlocks().map((block) => block.name); + expect(names).toContain('ve/paragraph'); + expect(names).toContain('ve/heading'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/inserter/__tests__/inserterRegistry.test.ts` around lines 181 - 190, The test currently only asserts fetchImpl was called; update it to also assert that no API blocks were added and built-in inserters remain registered after calling loadInserterBlocks with a non-ok response. After invoking loadInserterBlocks({ apiBase: ..., fetchImpl }), query the inserter registry (use the inserterRegistry/getAll/getRegisteredInserters helper used elsewhere in tests) to assert built-in inserters are still present and that no new blocks from the API exist, mirroring the assertions used in the "falls back to built-ins" test.resources/js/visual-editor/editor/rest/postRestClient.ts (1)
99-111: Hardenblocksvalidation before returningPostPayload.
isPostPayload()only checks thatblocksis an array. A malformed block entry still passes here and won't fail until store or render code touches it, which makes bad API data look like a client-side regression. Consider validating theBlockshape recursively at this boundary too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/rest/postRestClient.ts` around lines 99 - 111, The type guard isPostPayload currently only checks blocks is an array; add a recursive validation that each element matches the Block shape before returning PostPayload. Implement (or use) an isBlock(value: unknown): value is Block helper that verifies required Block properties and types (and nested fields if any), then change the blocks check in isPostPayload to Array.isArray(source.blocks) && source.blocks.every(isBlock). Keep the rest of the checks (id, title, updated_at) unchanged and ensure the new isBlock helper is exported/defined near isPostPayload so the guard reliably rejects malformed API data early.resources/js/visual-editor/editor/primitives/useInnerBlocksProps.tsx (1)
152-170: Duplicate offindBlockfromtreeUtils.
findBlockInStateduplicates thefindBlockfunction already exported from../store/treeUtils. Consider importing and reusing it to reduce code duplication.♻️ Reuse existing findBlock
import type { Block, EditorStore } from '../store'; -import { useChildren } from '../store'; +import { useChildren, findBlock } from '../store'; import { getBlock as getBlockDefinition, type BlockDefinition } from '../registry'; import { BlockContextProvider } from './BlockContext'; import { useEditorStore } from './EditorStoreContext'; // ... rest of file ... function useLiveBlock(store: EditorStore, clientId: string): Block | undefined { - return useStore(store, (state) => findBlockInState(state.blocks, clientId)); + return useStore(store, (state) => findBlock(state.blocks, clientId)); } - -function findBlockInState(blocks: Block[], clientId: string): Block | undefined { - for (const block of blocks) { - if (block.clientId === clientId) { - return block; - } - - const found = findBlockInState(block.innerBlocks, clientId); - - if (found !== undefined) { - return found; - } - } - - return undefined; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/primitives/useInnerBlocksProps.tsx` around lines 152 - 170, The function findBlockInState duplicates existing logic; remove findBlockInState and import the shared findBlock from ../store/treeUtils, then update useLiveBlock to call findBlock(store.state.blocks, clientId) (or pass the same parameters as the imported findBlock) so useLiveBlock uses the centralized findBlock implementation; ensure any type signatures match and update the import list accordingly.resources/js/visual-editor/editor/store/editorStore.ts (2)
160-196: No-op check could short-circuit earlier.The
targetIndex === currentIndexcheck at line 190 happens after array mutations. Moving it earlier avoids unnecessary work, though the current implementation is functionally correct.♻️ Optional: Early no-op detection
moveBlock: (clientId: string, location: MoveLocation) => { applyContentChange((state) => { const parentClientId = location.parentClientId ?? null; if (parentClientId !== null) { return null; } const currentIndex = state.blocks.findIndex( (block) => block.clientId === clientId ); if (currentIndex === -1) { return null; } + // Compute target index before mutation to enable early exit + const maxIndex = state.blocks.length - 1; + const targetIndex = + location.index < 0 + ? 0 + : location.index > maxIndex + ? maxIndex + : location.index; + + if (targetIndex === currentIndex) { + return null; + } + const block = state.blocks[currentIndex]; const withoutBlock = state.blocks.slice(); withoutBlock.splice(currentIndex, 1); - - const targetIndex = - location.index < 0 - ? 0 - : location.index > withoutBlock.length - ? withoutBlock.length - : location.index; - withoutBlock.splice(targetIndex, 0, block); - if (targetIndex === currentIndex) { - return null; - } - return { blocks: withoutBlock }; }, null); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/store/editorStore.ts` around lines 160 - 196, The moveBlock handler performs array splices before checking if the move is a no-op; inside moveBlock (within applyContentChange) compute currentIndex and targetIndex first and if targetIndex === currentIndex return null immediately to avoid creating withoutBlock and doing splice operations; update the logic in moveBlock so the early no-op check happens before creating withoutBlock or mutating the blocks array, keeping the rest of the behavior (returning { blocks: withoutBlock } and using parentClientId handling) unchanged.
62-66: Consider usingslicefor history bounds trimming.The
whileloop withshift()works but is less idiomatic. SinceHISTORY_LIMITis constant, you can compute the slice directly.♻️ Optional: Use slice for trimming
- const nextPast = history.past.concat([previousSnapshot]); - - while (nextPast.length > HISTORY_LIMIT) { - nextPast.shift(); - } + const combined = history.past.concat([previousSnapshot]); + const nextPast = combined.length > HISTORY_LIMIT + ? combined.slice(combined.length - HISTORY_LIMIT) + : combined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/store/editorStore.ts` around lines 62 - 66, The current history trimming builds nextPast via history.past.concat([previousSnapshot]) and then removes old entries with a while loop and shift(); replace that with a direct slice to keep only the most recent HISTORY_LIMIT entries: compute nextPast as history.past.concat([previousSnapshot]) and then set nextPast = nextPast.slice(-HISTORY_LIMIT) (or equivalent) so you avoid the loop and shifts; update references to nextPast, history.past, previousSnapshot and HISTORY_LIMIT accordingly.resources/js/visual-editor/editor/rest/useAutosave.ts (1)
204-217: Subscription conditions could be clearer.The two conditions (lines 209-211 and 214-216) handle the dirty-transition case and the edge case where the store is dirty but no save is pending. Consider consolidating with a comment explaining the intent.
♻️ Optional: Clarify subscription logic
const unsubscribe = store.subscribe((next, previous) => { if (disposed) { return; } - if (next.isDirty && !inFlight && !previous.isDirty) { - schedule(configRef.current.debounceMs); - return; - } - - if (next.isDirty && !inFlight && timerId === null) { + // Schedule a save when the store becomes dirty and no save is + // already pending (either in-flight or debounce-waiting). + const shouldSchedule = next.isDirty && !inFlight && timerId === null; + const justBecameDirty = next.isDirty && !previous.isDirty; + + if (shouldSchedule || (justBecameDirty && !inFlight)) { schedule(configRef.current.debounceMs); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/rest/useAutosave.ts` around lines 204 - 217, The subscription has two overlapping conditions in store.subscribe that both call schedule and are hard to read; consolidate them into a single clear condition and add a brief comment explaining intent: replace the two separate if blocks that check (next.isDirty && !inFlight && !previous.isDirty) and (next.isDirty && !inFlight && timerId === null) with one condition that schedules when next.isDirty && !inFlight && (previous.isDirty === false || timerId === null), keep the disposed early-return, and attach a comment above the combined condition explaining that we schedule a save when the store becomes dirty or when it remains dirty but no debounce timer is active.resources/js/visual-editor/editor/store/treeUtils.ts (1)
98-128: Consider avoiding intermediate array allocation.The
filter().map()chain creates an intermediate array. For large block trees, this could be optimized to a single pass usingreduceor a manual loop. However, given typical editor usage patterns, this is unlikely to be a bottleneck.♻️ Optional: Single-pass implementation
export function removeFromTree(blocks: Block[], clientId: string): Block[] { let changed = false; + const next: Block[] = []; - const next = blocks - .filter((block) => { - if (block.clientId === clientId) { - changed = true; - - return false; - } - - return true; - }) - .map((block) => { - if (block.innerBlocks.length === 0) { - return block; - } - - const nextInner = removeFromTree(block.innerBlocks, clientId); - - if (nextInner === block.innerBlocks) { - return block; - } - - changed = true; - - return { ...block, innerBlocks: nextInner }; - }); + for (const block of blocks) { + if (block.clientId === clientId) { + changed = true; + continue; + } + + if (block.innerBlocks.length === 0) { + next.push(block); + continue; + } + + const nextInner = removeFromTree(block.innerBlocks, clientId); + + if (nextInner === block.innerBlocks) { + next.push(block); + } else { + changed = true; + next.push({ ...block, innerBlocks: nextInner }); + } + } return changed ? next : blocks; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/store/treeUtils.ts` around lines 98 - 128, The current removeFromTree function creates an intermediate array via filter().map(); replace that two-pass approach with a single-pass loop or Array.prototype.reduce to avoid extra allocation: iterate over each block in blocks, if block.clientId === clientId skip pushing it and mark changed, otherwise recursively call removeFromTree on block.innerBlocks, if the returned nextInner differs from block.innerBlocks create a shallow-copied block with innerBlocks: nextInner (mark changed) and push it to the result array, else push the original block; at the end return changed ? resultArray : blocks. Keep the existing function name removeFromTree, parameters (blocks, clientId) and the changed flag semantics.resources/js/visual-editor/editor/blocks/shared/blockSplitMerge.ts (1)
121-135: HTML parsing via regex is fragile for edge cases.The
extractInnerHtmlandwrapInSameTagfunctions use regex to parse HTML, which may behave unexpectedly for attributes containing>, nested tags at the boundary, or self-closing elements. The fallback behavior (returning original/inner content) is safe, but consider using a DOM parser for robustness if edge cases arise in practice.♻️ Optional: DOM-based parsing for robustness
function extractInnerHtml(outerHtml: string): string { const template = document.createElement('template'); template.innerHTML = outerHtml.trim(); const firstChild = template.content.firstElementChild; return firstChild ? firstChild.innerHTML : outerHtml; } function wrapInSameTag(templateHtml: string, innerHtml: string): string { const template = document.createElement('template'); template.innerHTML = templateHtml.trim(); const firstChild = template.content.firstElementChild; if (!firstChild) { return innerHtml; } firstChild.innerHTML = innerHtml; return firstChild.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/blockSplitMerge.ts` around lines 121 - 135, The current extractInnerHtml and wrapInSameTag use fragile regex-based HTML parsing; replace their implementations to use a DOM template parser instead: in extractInnerHtml, create a template element, set template.innerHTML = outerHtml.trim(), get template.content.firstElementChild and return its .innerHTML or the original outerHtml as fallback; in wrapInSameTag, parse templateHtml into a template, get firstElementChild, if missing return innerHtml, otherwise set firstChild.innerHTML = innerHtml and return firstChild.outerHTML; update the functions named extractInnerHtml and wrapInSameTag accordingly to use this DOM-based approach and keep the existing fallback behavior.
🤖 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/blocks/heading/edit.tsx`:
- Around line 86-98: The effect that syncs attributes.level currently calls
editor.chain().focus().setNode('heading', { level }).run() and thus steals DOM
focus; update it to only run when this block is the active selection by gating
the effect with the existing isSelected guard (use the same pattern as the
earlier effect that reads isSelected) or replace the focusful call with a
non-focusing TipTap command (e.g., editor.commands.setNode or
editor.chain().setNode('heading', { level }).run() without .focus()) so the code
path inside the useEffect (which inspects editor.state.doc.firstChild and
compares first.attrs.level to level) only updates the node when isSelected is
true or when using a non-focusing API.
In `@resources/js/visual-editor/editor/blocks/shared/useBlockTiptap.ts`:
- Around line 74-89: The keydown handler handleKeyDown should ignore IME
composition and modifier-key shortcuts: add an early return false if
event.isComposing or any modifier keys are pressed (event.ctrlKey ||
event.metaKey || event.altKey) before checking Enter/Backspace, so
onEnterRef.current() and onBackspaceRef.current() are not invoked during
composition or when Ctrl/Cmd/Alt are held; keep the existing Enter (and
shiftKey) and Backspace logic unchanged otherwise.
In `@resources/js/visual-editor/editor/components/Canvas.tsx`:
- Line 14: The aria-label value "Editor canvas" is hardcoded; update the Canvas
component to use a localized string instead—inject a translation prop or use the
existing i18n utility/context and replace the literal aria-label with the
localized value (e.g., using a prop like canvasLabel or a translation function
such as t('editor.canvas')). Ensure the Canvas component (or its parent)
accepts/provides the localized label and that the aria-label attribute uses that
variable rather than the hardcoded English string.
In `@resources/js/visual-editor/editor/components/editor.css`:
- Around line 111-113: The .ve-richtext rule removes the focus outline and
leaves keyboard users without a visible cue; restore an accessible focus
indicator by removing outline: none and adding a focus state such as
.ve-richtext:focus, .ve-richtext:focus-visible { outline: 2px solid Highlight;
outline-offset: 2px; } (or outline: auto) so the editable surface shows a clear
focus ring when focused. Ensure you edit the .ve-richtext selector (and add the
:focus/:focus-visible rules) in the same stylesheet so the caret/keyboard focus
is always visually indicated.
In `@resources/js/visual-editor/editor/inserter/insertBlockActions.ts`:
- Around line 21-24: The object spread currently allows factory() to override
the generated clientId (newBlock built with clientId: createClientId(),
...factory()), so change the merge order to spread factory() first and then set
clientId: createClientId() (i.e., {...factory(), clientId: createClientId()}) so
the generated id cannot be overridden; apply the same ordering fix to the other
block-construction site that mirrors the constructor logic referenced in the
file (the second factory/constructor that currently spreads factory() after
clientId).
In `@resources/js/visual-editor/editor/main.tsx`:
- Line 62: The call to loadInserterBlocks({ apiBase: config.apiBase }) is
currently invoked with void which discards its Promise and allows unhandled
rejections; update the caller in main.tsx to handle rejections by either
awaiting the call inside an async init function with try/catch or attaching a
.catch(...) to the returned promise and log or surface the error (use
processLogger/console.error or the existing logging utility) so failures
fetching from the REST endpoint are handled gracefully and include context
(e.g., "loadInserterBlocks failed" and the error).
In `@resources/js/visual-editor/editor/primitives/BlockContext.tsx`:
- Around line 54-57: The cache currently only compares lastRawRef.current to
raw, so changing the guard function can leave lastResultRef.current stale; add a
lastGuardRef (useRef) and include a comparison of lastGuardRef.current to guard
in the cache invalidation check, updating lastGuardRef.current whenever you
update lastRawRef/current and re-evaluating lastResultRef.current by calling
guard(raw) so the cached result reflects both raw and the current guard
function.
In `@resources/js/visual-editor/editor/rest/postRestClient.ts`:
- Around line 145-152: fetchPost and savePost currently call fetcher() directly
so network failures throw raw errors; wrap the fetcher(...) invocation in each
of these functions (references: fetchPost, savePost, fetcher,
handlePostResponse) in a try-catch, rethrow if the caught error is an AbortError
(preserve cancellation via options.signal), otherwise throw/return a normalized
PostRestError that includes the original error details so callers only receive
PostRestError for pre-response failures; ensure existing response handling still
calls handlePostResponse(response) after a successful fetch.
In `@src/Models/VisualEditorPost.php`:
- Line 26: Replace the open mass-assignment on the VisualEditorPost model by
removing protected $guarded = [] and declaring a protected $fillable array that
lists only the allowed attributes (e.g., title, body/content, slug,
published_at, any editor-specific fields) — explicitly exclude author_id so it
cannot be mass-assigned; update any tests or factory usage that passed author_id
via create() to set the post author server-side (use relationship assignment or
a factory state) instead of mass assignment.
In `@tests/Feature/VisualEditor/PostsControllerTest.php`:
- Around line 23-34: The helper actingAsUser() always creates a TestUser with
the same email causing unique constraint failures; update the TestUser::create
call in actingAsUser to generate a unique email on each invocation (e.g., append
a timestamp/uniqid/random string or use a factory that ensures uniqueness) and
keep the test()->actingAs($user) behavior unchanged so each test receives a
distinct user email.
In `@vite.config.ts`:
- Around line 27-33: The build config is externalizing React packages in the
external array (entries: 'react', 'react/jsx-runtime', 'react/jsx-dev-runtime',
'react-dom', 'react-dom/client'), which produces unresolved bare imports at
runtime; remove those entries from the external list in vite.config.ts so the
bundler includes React in the app bundle, or alternatively implement a runtime
import resolution (e.g., provide an import map or module resolver) if you
intentionally want them externalized—update the external array or add the
import-map/module-resolution logic accordingly.
---
Nitpick comments:
In `@database/migrations/2026_04_14_000000_create_ve_contents_table.php`:
- Line 29: Replace the nullable unsignedBigInteger column definition for
author_id in the CreateVeContentsTable migration with a nullable foreign key
that references users and uses nullOnDelete; specifically update the schema
change around the author_id definition in the CreateVeContentsTable migration
(the ve_contents table setup) to use a foreignId constrained to 'users' with
nullOnDelete so deleted users set author_id to null and prevent orphaned
ownership records.
In `@resources/js/visual-editor/editor/blocks/paragraph/edit.tsx`:
- Around line 111-138: The callbacks passed into useBlockTiptap (onEnter,
onBackspaceAtStart) reference the editor variable returned from useBlockTiptap
which can look like a forward reference to TypeScript; either capture the editor
in a stable ref and use that ref.current inside onEnter/onBackspaceAtStart, or
rewrite the callbacks to receive the editor instance (or a guaranteed non-null
typed editor) before calling handleBlockEnter/handleBlockBackspace; update
references to useBlockTiptap, onEnter, onBackspaceAtStart, editor,
handleBlockEnter, handleBlockBackspace, slashActiveRef, selectSlashBlockAtIndex,
filteredRef, slashIndexRef, store and clientId accordingly, or alternatively add
a concise inline comment explaining why the current pattern is safe and assert
the editor type to satisfy TypeScript.
In `@resources/js/visual-editor/editor/blocks/shared/blockSplitMerge.ts`:
- Around line 121-135: The current extractInnerHtml and wrapInSameTag use
fragile regex-based HTML parsing; replace their implementations to use a DOM
template parser instead: in extractInnerHtml, create a template element, set
template.innerHTML = outerHtml.trim(), get template.content.firstElementChild
and return its .innerHTML or the original outerHtml as fallback; in
wrapInSameTag, parse templateHtml into a template, get firstElementChild, if
missing return innerHtml, otherwise set firstChild.innerHTML = innerHtml and
return firstChild.outerHTML; update the functions named extractInnerHtml and
wrapInSameTag accordingly to use this DOM-based approach and keep the existing
fallback behavior.
In `@resources/js/visual-editor/editor/components/EditorShell.tsx`:
- Around line 140-153: The comment in shouldDeferToNativeHistory is misleading
about <select> elements implying they have native undo/redo; update the
explanatory comment around shouldDeferToNativeHistory (and the sentence
mentioning `<select>`) to state that non-contenteditable focusable controls
(e.g., <select>) should preserve their native keyboard behavior even though they
do not provide native undo/redo, and clarify that only contenteditable Tiptap
surfaces route undo/redo through the store; leave the logic (return false for
other elements) unchanged.
In `@resources/js/visual-editor/editor/components/Icon.tsx`:
- Around line 11-19: The component currently treats whitespace-only strings as
valid titles; update the title handling by trimming before checking length:
change hasTitle to use title.trim() (e.g., hasTitle = typeof title === 'string'
&& title.trim().length > 0), pass the trimmed title to FontAwesomeIcon when
hasTitle is true (title={hasTitle ? title.trim() : undefined}), and keep role
and aria-hidden logic based on this new hasTitle so whitespace-only titles are
treated as missing.
In `@resources/js/visual-editor/editor/components/SlashCommandPopover.tsx`:
- Around line 16-22: The popover's markup in SlashCommandPopover lacks ARIA
listbox semantics; update the rendered list container (the <ul> in the
component) to include role="listbox" and each list item (rendered options in the
component's map) to include role="option" and an appropriate aria-selected
attribute reflecting selection state (true/false); ensure you update the render
logic in the SlashCommandPopover component where the option elements are created
so screen readers can navigate the list properly.
In
`@resources/js/visual-editor/editor/inserter/__tests__/inserterRegistry.test.ts`:
- Around line 181-190: The test currently only asserts fetchImpl was called;
update it to also assert that no API blocks were added and built-in inserters
remain registered after calling loadInserterBlocks with a non-ok response. After
invoking loadInserterBlocks({ apiBase: ..., fetchImpl }), query the inserter
registry (use the inserterRegistry/getAll/getRegisteredInserters helper used
elsewhere in tests) to assert built-in inserters are still present and that no
new blocks from the API exist, mirroring the assertions used in the "falls back
to built-ins" test.
In `@resources/js/visual-editor/editor/primitives/useInnerBlocksProps.tsx`:
- Around line 152-170: The function findBlockInState duplicates existing logic;
remove findBlockInState and import the shared findBlock from ../store/treeUtils,
then update useLiveBlock to call findBlock(store.state.blocks, clientId) (or
pass the same parameters as the imported findBlock) so useLiveBlock uses the
centralized findBlock implementation; ensure any type signatures match and
update the import list accordingly.
In `@resources/js/visual-editor/editor/rest/__tests__/postRestClient.test.ts`:
- Around line 93-132: The savePost tests are missing a CSRF-header assertion;
update the 'PUTs the block tree and returns the persisted payload' spec for
savePost to also assert the fetch call includes the same CSRF header checked in
fetchPost (e.g. ensure (init.headers as Headers).get('X-WP-Nonce') (or the
project’s CSRF header name) is present and equals the expected test nonce value
provided by the test setup), locating the call via the savePost test's
fetchImpl.mock.calls and adding the header assertion alongside the existing
method/content-type/body checks.
In `@resources/js/visual-editor/editor/rest/__tests__/useAutosave.test.tsx`:
- Around line 51-53: The test currently calls onState(state) during render
(inside the render path where onState is checked), which makes the harness
impure; change this by removing the direct call from render and invoking onState
from a React effect instead: create a useEffect (or the test harness equivalent)
that runs when state (and onState) change, and inside it guard if (onState)
onState(state); include onState and state in the effect dependency array so
notifications occur on updates and avoid invoking callbacks during render.
In `@resources/js/visual-editor/editor/rest/postRestClient.ts`:
- Around line 99-111: The type guard isPostPayload currently only checks blocks
is an array; add a recursive validation that each element matches the Block
shape before returning PostPayload. Implement (or use) an isBlock(value:
unknown): value is Block helper that verifies required Block properties and
types (and nested fields if any), then change the blocks check in isPostPayload
to Array.isArray(source.blocks) && source.blocks.every(isBlock). Keep the rest
of the checks (id, title, updated_at) unchanged and ensure the new isBlock
helper is exported/defined near isPostPayload so the guard reliably rejects
malformed API data early.
In `@resources/js/visual-editor/editor/rest/useAutosave.ts`:
- Around line 204-217: The subscription has two overlapping conditions in
store.subscribe that both call schedule and are hard to read; consolidate them
into a single clear condition and add a brief comment explaining intent: replace
the two separate if blocks that check (next.isDirty && !inFlight &&
!previous.isDirty) and (next.isDirty && !inFlight && timerId === null) with one
condition that schedules when next.isDirty && !inFlight && (previous.isDirty ===
false || timerId === null), keep the disposed early-return, and attach a comment
above the combined condition explaining that we schedule a save when the store
becomes dirty or when it remains dirty but no debounce timer is active.
In `@resources/js/visual-editor/editor/store/editorStore.ts`:
- Around line 160-196: The moveBlock handler performs array splices before
checking if the move is a no-op; inside moveBlock (within applyContentChange)
compute currentIndex and targetIndex first and if targetIndex === currentIndex
return null immediately to avoid creating withoutBlock and doing splice
operations; update the logic in moveBlock so the early no-op check happens
before creating withoutBlock or mutating the blocks array, keeping the rest of
the behavior (returning { blocks: withoutBlock } and using parentClientId
handling) unchanged.
- Around line 62-66: The current history trimming builds nextPast via
history.past.concat([previousSnapshot]) and then removes old entries with a
while loop and shift(); replace that with a direct slice to keep only the most
recent HISTORY_LIMIT entries: compute nextPast as
history.past.concat([previousSnapshot]) and then set nextPast =
nextPast.slice(-HISTORY_LIMIT) (or equivalent) so you avoid the loop and shifts;
update references to nextPast, history.past, previousSnapshot and HISTORY_LIMIT
accordingly.
In `@resources/js/visual-editor/editor/store/treeUtils.ts`:
- Around line 98-128: The current removeFromTree function creates an
intermediate array via filter().map(); replace that two-pass approach with a
single-pass loop or Array.prototype.reduce to avoid extra allocation: iterate
over each block in blocks, if block.clientId === clientId skip pushing it and
mark changed, otherwise recursively call removeFromTree on block.innerBlocks, if
the returned nextInner differs from block.innerBlocks create a shallow-copied
block with innerBlocks: nextInner (mark changed) and push it to the result
array, else push the original block; at the end return changed ? resultArray :
blocks. Keep the existing function name removeFromTree, parameters (blocks,
clientId) and the changed flag semantics.
In `@routes/web.php`:
- Around line 7-13: The route closure for Route::get('/editor',
...)->name('visual-editor.editor') currently passes hardcoded view params
'postId' => '1' and 'postType' => 'page'; change it to accept dynamic inputs
(e.g., route parameters like {post} and {type} or read from query string) and
forward those values into the view 'visual-editor::editor.mount' instead of the
hardcoded literals, validating/casting the inputs as needed (or mark the route
clearly as a temporary dev-only entry in comments/config if you intend to keep
hardcoded defaults). Ensure you update any callers/links to use the new
parameters and keep the route name 'visual-editor.editor' unchanged.
In `@src/Policies/VisualEditorPostPolicy.php`:
- Around line 84-91: The method VisualEditorPostPolicy::isOwnerOrAdmin only
checks ownership, so rename it to isOwner (or implement real admin-checking if
admin behavior is desired); update all call sites that invoke isOwnerOrAdmin
(e.g., the two places that currently call it in this policy) to call isOwner
instead, and update any PHPDoc/type hints and tests referencing isOwnerOrAdmin
to use the new name so behavior and naming stay consistent.
In `@tests/Feature/EditorMountViewTest.php`:
- Around line 14-15: The test reads view files using file_get_contents into
$contents but doesn't assert the read succeeded, so failures are unclear when
file_get_contents returns false; update EditorMountViewTest to assert the read
before further assertions (e.g. use $this->assertNotFalse($contents) or
$this->assertIsString($contents) and optionally
$this->assertNotEmpty($contents)) immediately after each file_get_contents call
(references: variable $contents and the file_get_contents(...) calls present in
the test at the multiple spots noted) so subsequent assertions operate on a
valid string.
🪄 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: 45d8fe12-4033-43d0-b00f-0c0cb6a507a8
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lock,!composer.lockpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (122)
.gitignoreREACT-MIGRATION.mdcomposer.jsondatabase/migrations/2026_04_14_000000_create_ve_contents_table.phpdatabase/migrations/testing/2026_04_14_000000_create_users_table.phpdocs/phase-1-dnd-kit-findings.mddocs/phase-1-rest-api.mddocs/phase-1-tiptap-strategy.mdindex.htmlpackage.jsonresources/js/editor-spike/App.tsxresources/js/editor-spike/__tests__/BlockContext.test.tsxresources/js/editor-spike/__tests__/mocks.test.tsresources/js/editor-spike/__tests__/paragraph.test.tsxresources/js/editor-spike/__tests__/postTitle.test.tsxresources/js/editor-spike/__tests__/queryLoop.test.tsxresources/js/editor-spike/__tests__/smoke.test.tsxresources/js/editor-spike/__tests__/useInnerBlocksProps.test.tsxresources/js/editor-spike/blocks/paragraph/edit.tsxresources/js/editor-spike/blocks/post-title/edit.tsxresources/js/editor-spike/blocks/post-title/index.tsresources/js/editor-spike/blocks/query-loop/edit.tsxresources/js/editor-spike/blocks/query-loop/index.tsresources/js/editor-spike/index.cssresources/js/editor-spike/main.tsxresources/js/editor-spike/mocks/blockTree.tsresources/js/editor-spike/mocks/mockApi.tsresources/js/editor-spike/mocks/posts.tsresources/js/editor-spike/primitives/BlockContext.tsxresources/js/editor-spike/primitives/BlockTreeContext.tsxresources/js/editor-spike/primitives/useInnerBlocksProps.tsxresources/js/editor-spike/richtext/useTiptap.tsresources/js/visual-editor/editor/blocks/__tests__/heading.test.tsxresources/js/visual-editor/editor/blocks/__tests__/paragraph.test.tsxresources/js/visual-editor/editor/blocks/__tests__/slashCommand.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/BlockList.tsxresources/js/visual-editor/editor/components/BlockWrapper.tsxresources/js/visual-editor/editor/components/Canvas.tsxresources/js/visual-editor/editor/components/EditorBoot.tsxresources/js/visual-editor/editor/components/EditorShell.tsxresources/js/visual-editor/editor/components/Icon.tsxresources/js/visual-editor/editor/components/InserterPanel.tsxresources/js/visual-editor/editor/components/RichTextToolbar.tsxresources/js/visual-editor/editor/components/SlashCommandPopover.tsxresources/js/visual-editor/editor/components/__tests__/BlockList.test.tsxresources/js/visual-editor/editor/components/__tests__/BlockListDnd.test.tsxresources/js/visual-editor/editor/components/__tests__/BlockListDndDispatch.test.tsxresources/js/visual-editor/editor/components/__tests__/BlockWrapper.test.tsxresources/js/visual-editor/editor/components/__tests__/EditorBoot.test.tsxresources/js/visual-editor/editor/components/__tests__/EditorShell.test.tsxresources/js/visual-editor/editor/components/__tests__/InserterPanel.test.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/index.htmlresources/js/visual-editor/editor/inserter/__tests__/insertBlockActions.test.tsresources/js/visual-editor/editor/inserter/__tests__/inserterRegistry.test.tsresources/js/visual-editor/editor/inserter/builtinInserterBlocks.tsresources/js/visual-editor/editor/inserter/filterInserterBlocks.tsresources/js/visual-editor/editor/inserter/index.tsresources/js/visual-editor/editor/inserter/insertBlockActions.tsresources/js/visual-editor/editor/inserter/inserterRegistry.tsresources/js/visual-editor/editor/inserter/loadInserterBlocks.tsresources/js/visual-editor/editor/main.tsxresources/js/visual-editor/editor/primitives/BlockContext.tsxresources/js/visual-editor/editor/primitives/EditorStoreContext.tsxresources/js/visual-editor/editor/primitives/ReadOnlyContext.tsxresources/js/visual-editor/editor/primitives/__tests__/BlockContext.test.tsxresources/js/visual-editor/editor/primitives/__tests__/EditorStoreContext.test.tsxresources/js/visual-editor/editor/primitives/__tests__/useBlockPreview.test.tsxresources/js/visual-editor/editor/primitives/__tests__/useInnerBlocksProps.test.tsxresources/js/visual-editor/editor/primitives/index.tsresources/js/visual-editor/editor/primitives/useBlockPreview.tsxresources/js/visual-editor/editor/primitives/useInnerBlocksProps.tsxresources/js/visual-editor/editor/registry/blockRegistry.tsresources/js/visual-editor/editor/registry/index.tsresources/js/visual-editor/editor/rest/__tests__/postRestClient.test.tsresources/js/visual-editor/editor/rest/__tests__/useAutosave.test.tsxresources/js/visual-editor/editor/rest/index.tsresources/js/visual-editor/editor/rest/postRestClient.tsresources/js/visual-editor/editor/rest/useAutosave.tsresources/js/visual-editor/editor/store/__tests__/editorStore.test.tsresources/js/visual-editor/editor/store/__tests__/editorStoreHistory.test.tsresources/js/visual-editor/editor/store/__tests__/editorStoreSelectors.test.tsxresources/js/visual-editor/editor/store/editorStore.tsresources/js/visual-editor/editor/store/ids.tsresources/js/visual-editor/editor/store/index.tsresources/js/visual-editor/editor/store/treeUtils.tsresources/js/visual-editor/editor/store/types.tsresources/js/visual-editor/editor/test-setup.tsresources/views/editor-spike.blade.phpresources/views/editor/mount.blade.phproutes/api.phproutes/web.phpsrc/Http/Controllers/VisualEditorBlocksController.phpsrc/Http/Controllers/VisualEditorPostsController.phpsrc/Http/Requests/UpdatePostBlocksRequest.phpsrc/Models/VisualEditorPost.phpsrc/Policies/VisualEditorPostPolicy.phpsrc/Registries/BlockTypeRegistry.phpsrc/Rules/BlockTreeRule.phpsrc/VisualEditorServiceProvider.phptests/Feature/EditorMountViewTest.phptests/Feature/VisualEditor/BlocksControllerTest.phptests/Feature/VisualEditor/PostsControllerTest.phptests/Pest.phptests/TestCase.phptests/TestUser.phptests/Unit/VisualEditor/BlockTypeRegistryTest.phptsconfig.jsonvite.config.tsvitest.config.ts
💤 Files with no reviewable changes (24)
- resources/js/editor-spike/tests/smoke.test.tsx
- resources/js/editor-spike/index.css
- resources/js/editor-spike/mocks/blockTree.ts
- resources/views/editor-spike.blade.php
- resources/js/editor-spike/main.tsx
- resources/js/editor-spike/tests/mocks.test.ts
- index.html
- resources/js/editor-spike/blocks/post-title/edit.tsx
- resources/js/editor-spike/tests/queryLoop.test.tsx
- resources/js/editor-spike/blocks/paragraph/edit.tsx
- resources/js/editor-spike/mocks/posts.ts
- resources/js/editor-spike/tests/paragraph.test.tsx
- resources/js/editor-spike/blocks/query-loop/edit.tsx
- resources/js/editor-spike/primitives/useInnerBlocksProps.tsx
- resources/js/editor-spike/blocks/query-loop/index.ts
- resources/js/editor-spike/blocks/post-title/index.ts
- resources/js/editor-spike/mocks/mockApi.ts
- resources/js/editor-spike/primitives/BlockTreeContext.tsx
- resources/js/editor-spike/richtext/useTiptap.ts
- resources/js/editor-spike/tests/postTitle.test.tsx
- resources/js/editor-spike/primitives/BlockContext.tsx
- resources/js/editor-spike/tests/BlockContext.test.tsx
- resources/js/editor-spike/tests/useInnerBlocksProps.test.tsx
- resources/js/editor-spike/App.tsx
# Conflicts: # .gitignore
- heading/edit.tsx: drop .focus() from level-sync effect so changing
a heading level no longer steals DOM focus from unrelated blocks
- useBlockTiptap: ignore IME composition and Ctrl/Meta/Alt modifier
chords in the block-level keydown handler so Enter/Backspace only
split/merge blocks on actual user intent
- Canvas: accept an optional label prop (default "Editor canvas")
so the aria-label can be localized once an i18n pipeline lands
- editor.css: add .ve-richtext:focus-visible outline so keyboard
users get a visible focus indicator on the contenteditable
- insertBlockActions: spread factory() before clientId so the
generated id cannot be overridden by factory output (applies to
both insertBlockAtSelection and replaceBlockWithInserterBlock)
- main.tsx: attach .catch() to loadInserterBlocks so REST failures
are logged instead of becoming unhandled rejections
- BlockContext.useBlockContextValue: include guard identity in the
cache invalidation check so a new guard re-evaluates the cached
value even when raw is unchanged
- postRestClient: wrap fetcher() in fetchPost/savePost with a
normalizer that rethrows AbortError but converts other network
failures into PostRestError with the original error preserved
- VisualEditorPost: replace $guarded = [] with an explicit
$fillable list ('title', 'blocks') so author_id cannot be
mass-assigned from request input
- PostsControllerTest: set author_id via direct property
assignment in the createPost helper now that it is non-fillable,
and unique-ify the actingAsUser email via uniqid() to guard
against any future collisions
- vite.config.ts: drop the React externals so the standalone build
produces a self-contained bundle (the host app's Vite continues
to bundle main.tsx through @Vite for the dev-app flow)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
resources/js/visual-editor/editor/primitives/BlockContext.tsx (1)
37-43: Clarify stability contract in the hook docs.The doc says stability only depends on context value changes, but the implementation also invalidates when guard identity changes. Consider updating this comment so usage expectations are explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/editor/primitives/BlockContext.tsx` around lines 37 - 43, Update the JSDoc for the hook in BlockContext.tsx to state that the returned reference is stable across renders until the underlying context value changes or the runtime type guard function identity changes; mention that changing the guard (the function passed to the hook) will cause the memoized reference to be invalidated and consumers may re-run, and reference BlockContextProvider and the guard parameter by name so callers know both inputs affect stability.
🤖 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/main.tsx`:
- Around line 29-33: The placeholder fallback label in function PlaceholderEdit
is hardcoded; change it to use the WordPress i18n translate helper (e.g., __
from '@wordpress/i18n' or wp.i18n.__) so the default 'Placeholder block' goes
through localization; update the import to include the translate function and
replace the literal in the ternary (attributes.label fallback) with a call like
__('Placeholder block', 'your-text-domain') keeping the rest of PlaceholderEdit
unchanged.
In `@resources/js/visual-editor/editor/rest/postRestClient.ts`:
- Around line 3-8: The PostPayload interface currently types updated_at as
string which is incompatible with the API transformer that can emit null; update
the type of updated_at to allow null (e.g., string | null) and update any other
similar interfaces or occurrences (the other declaration referenced at lines
106-111) so client-side validation accepts the API's nullable updated_at values;
search for PostPayload and the other affected interfaces and change their
updated_at types accordingly.
---
Nitpick comments:
In `@resources/js/visual-editor/editor/primitives/BlockContext.tsx`:
- Around line 37-43: Update the JSDoc for the hook in BlockContext.tsx to state
that the returned reference is stable across renders until the underlying
context value changes or the runtime type guard function identity changes;
mention that changing the guard (the function passed to the hook) will cause the
memoized reference to be invalidated and consumers may re-run, and reference
BlockContextProvider and the guard parameter by name so callers know both inputs
affect stability.
🪄 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: 12fe0088-5507-4887-8b0b-65cf50108dc0
📒 Files selected for processing (11)
resources/js/visual-editor/editor/blocks/heading/edit.tsxresources/js/visual-editor/editor/blocks/shared/useBlockTiptap.tsresources/js/visual-editor/editor/components/Canvas.tsxresources/js/visual-editor/editor/components/editor.cssresources/js/visual-editor/editor/inserter/insertBlockActions.tsresources/js/visual-editor/editor/main.tsxresources/js/visual-editor/editor/primitives/BlockContext.tsxresources/js/visual-editor/editor/rest/postRestClient.tssrc/Models/VisualEditorPost.phptests/Feature/VisualEditor/PostsControllerTest.phpvite.config.ts
✅ Files skipped from review due to trivial changes (1)
- resources/js/visual-editor/editor/components/editor.css
🚧 Files skipped from review as they are similar to previous changes (6)
- resources/js/visual-editor/editor/components/Canvas.tsx
- vite.config.ts
- tests/Feature/VisualEditor/PostsControllerTest.php
- resources/js/visual-editor/editor/blocks/heading/edit.tsx
- resources/js/visual-editor/editor/inserter/insertBlockActions.ts
- resources/js/visual-editor/editor/blocks/shared/useBlockTiptap.ts
Comment on lines
+29
to
+33
| function PlaceholderEdit({ attributes }: BlockEditProps) { | ||
| const label = | ||
| typeof attributes.label === 'string' ? attributes.label : 'Placeholder block'; | ||
|
|
||
| return <p className="ve-placeholder-block">{label}</p>; |
There was a problem hiding this comment.
Localize the placeholder fallback label.
'Placeholder block' is user-visible (Line 31) and currently hardcoded. This should go through the same localization path as other editor UI strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/js/visual-editor/editor/main.tsx` around lines 29 - 33, The
placeholder fallback label in function PlaceholderEdit is hardcoded; change it
to use the WordPress i18n translate helper (e.g., __ from '@wordpress/i18n' or
wp.i18n.__) so the default 'Placeholder block' goes through localization; update
the import to include the translate function and replace the literal in the
ternary (attributes.label fallback) with a call like __('Placeholder block',
'your-text-domain') keeping the rest of PlaceholderEdit unchanged.
VisualEditorPostsController emits $post->updated_at?->toIso8601String(), which is string|null. The client-side PostPayload interface and isPostPayload guard were narrower (string only), so a nullable timestamp would fail validation at handlePostResponse and surface as a "did not match expected post shape" error. Widen the type to string|null and accept null in the guard. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rest/postRestClient.ts`:
- Around line 99-111: The blocks array check in isPostPayload is too loose and
allows malformed block entries into state; update isPostPayload (and PostPayload
validation) to ensure source.blocks is an array and every element is a non-null
object with the specific properties and types your store expects (e.g., each
block has numeric id, string type, and the fields the editorStore uses for block
operations), e.g. replace the Array.isArray(source.blocks) check with a
source.blocks.every(...) predicate that asserts required keys and types so
invalid blocks are rejected before being written to editorStore state.
🪄 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: 7212eee0-3c20-4076-9654-f5b95c78c0f9
📒 Files selected for processing (1)
resources/js/visual-editor/editor/rest/postRestClient.ts
isPostPayload only checked Array.isArray(source.blocks), so a malformed block (missing clientId/name, non-object attributes, non-array innerBlocks) would pass the REST response guard and land in the editor store, where store actions assume Block invariants. Add isBlockShape as a recursive type guard that asserts each of the four Block fields and walks innerBlocks, and call it from isPostPayload via source.blocks.every(isBlockShape). Cover the new path with a fetchPost test whose payload has a valid outer block but an invalid nested innerBlock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
This was referenced Apr 16, 2026
18 tasks done
12 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Phase 1 of the React editor migration lands on the long-running
reactbranch. This brings the React editor from an empty shell to a working authoring surface: Zustand store, undo/redo, dnd-kit reordering, paragraph/heading blocks with a shared rich-text toolbar, a sidebar inserter with slash command, REST load/save with autosave, and the Blade mount view that boots it. The Alpine editor remnants and Phase 0 spike harness have been removed.Closes: Phase 1 scope (issues #264–#276, #290)
Type of Change
Motivation and Context
`add/phase-one` is the integration branch where every Phase 1 merge request has been landing. Now that the last item (Alpine cleanup, #290) is merged and CodeRabbit findings on this branch have been addressed, the accumulated phase is ready to roll up into `react` as a single reviewable unit before Phase 2 work starts.
Changes Made
How Has This Been Tested?
Testing Environment:
Tests Performed:
Accessibility Tests Run
Details: Individual phase 1 MRs covered a11y for their components (toolbar, inserter, shell). A full cross-cutting a11y sweep is planned for a later phase once the editor surface stabilizes.
Tests Added
Test details: Each sub-MR landed its own unit + feature tests; totals above reflect the suite on this branch.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores