Skip to content

fix(mutator): keep moved blocks inside the destination wrapper's markup - #40

Merged
zackkatz merged 1 commit into
developfrom
fix/mutator-move-placeholder
Jul 6, 2026
Merged

zackkatz merged 1 commit into
developfrom
fix/mutator-move-placeholder

Conversation

@zackkatz

@zackkatz zackkatz commented Jul 6, 2026 •

Copy link
Copy Markdown
Member

The bug

edit_block_tree's move op corrupted saved content in two ordinary scenarios, serializing the moved block outside the destination wrapper's markup (…</div><!-- wp:paragraph -->…), which the editor then flags as invalid content:

  1. Move into a childless container — its innerContent parses back as a single unsplit wrapper string (['<div class="wp-block-group"></div>']), so the Nth-null scan finds nothing and appends the placeholder after the closing tag.
  2. Move to a container's end position — destination index == child count, so no Nth null exists; same fall-through, same corruption.

insert-child was hardened against exactly these shapes (the BLOCK-20 work); move's destination handling never got the guards. Root cause: duplicated placeholder-splice logic, fixed in one copy.

The fix

Extract the guarded logic into one shared insert_child_placeholders() helper (wrapper-split normalization + Nth-null scan + before-closing-string fallback) and route both insert-child and move through it. Net: −95/+203 including tests; the op-facing behavior of insert-child is unchanged (its full regression suite passes through the helper).

Evidence

  • Both new regression tests fail pre-fix (placeholder lands at/after the closing string) and pass post-fix, asserting on the serialized markup ordering.
  • composer lint 0/0 · composer analyze [OK] · PHPUnit 1100 single-site + 24 yoast + 5 multisite green.

https://claude.ai/code/session_01B5BxSkbRy8w6hzuuNhJqn2

Summary by CodeRabbit

  • Bug Fixes
    • Moving blocks into containers now keeps them inside the container markup, including empty containers and last-position moves.
    • Inserting child blocks now places placeholders more reliably, preventing content from appearing outside the intended wrapper.
  • Tests
    • Added coverage for moving content into self-closing wrappers and into the end of a container.

💾 Build file (9488bb3).

The move op's destination placeholder used a bare Nth-null scan, so two
ordinary moves corrupted saved content by appending the null AFTER the
closing-tag string — serializing the moved block OUTSIDE the wrapper:
(1) moving into a childless container, whose innerContent parses back as
a single unsplit wrapper string, and (2) moving to a container's end
position, where no Nth null exists. insert-child already guards both
shapes; the guards now live in one shared insert_child_placeholders()
helper used by both ops.

Regression tests fail pre-fix on both shapes and assert the serialized
markup places the moved block between the wrapper's tags.

Claude-Session: https://claude.ai/code/session_01B5BxSkbRy8w6hzuuNhJqn2
@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Refactors Block_Mutator::mutate() insert-child and move operations to use a new shared private helper, insert_child_placeholders(), which splits unsplit wrapper innerContent and inserts null placeholders at the correct index. Adds two regression tests and a changelog entry.

Changes

Placeholder Insertion Refactor

Layer / File(s) Summary
Shared placeholder insertion helper
wordpress-plugin/gk-block-mcp/includes/class-block-mutator.php
New private method insert_child_placeholders() splits an unsplit wrapper string at the first > when no null placeholders exist, then inserts $count null placeholders at the position corresponding to $child_position, falling back to inserting before the last piece if the Nth null can't be found.
Wire helper into insert-child and move
wordpress-plugin/gk-block-mcp/includes/class-block-mutator.php
insert-child computes a normalized child_position and normalizes innerContent to an array before calling the helper; move replaces inline null_seen/array_splice logic with a call to the helper using $dest_index and $count.
Regression tests and changelog
wordpress-plugin/gk-block-mcp/tests/Block/BlockMutatorTest.php, wordpress-plugin/gk-block-mcp/readme.txt
Adds tests verifying moved blocks nest inside self-closing wrapper containers and at container end positions, plus a changelog entry describing the fix for blocks previously saved outside container markup.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Block_Mutator
  participant insert_child_placeholders

  Caller->>Block_Mutator: mutate(insert-child or move)
  Block_Mutator->>Block_Mutator: normalize innerContent to array
  Block_Mutator->>insert_child_placeholders: child_position or dest_index, count
  insert_child_placeholders->>insert_child_placeholders: split wrapper if unsplit
  insert_child_placeholders->>insert_child_placeholders: locate Nth null or fallback to last piece
  insert_child_placeholders-->>Block_Mutator: updated innerContent with placeholders
  Block_Mutator-->>Caller: mutated block
Loading

Possibly related PRs

  • GravityKit/block-mcp#17: Both PRs update Block_Mutator::mutate() to normalize container innerContent for insert-child by splitting wrapper strings and inserting null placeholders, with companion test coverage.
  • GravityKit/block-mcp#18: Both PRs modify Block_Mutator::mutate() to normalize unsplit wrapper innerContent and insert null placeholders so children nest inside the wrapper rather than serializing as siblings.
  • GravityKit/block-mcp#13: Both PRs address container wrapper innerContent/innerBlocks handling by splitting wrapper HTML and inserting per-child null placeholders.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main fix: moved blocks now stay inside the destination wrapper's markup.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mutator-move-placeholder

Comment @coderabbitai help to get the list of available commands.

@zackkatz
zackkatz merged commit ec8f1dd into develop Jul 6, 2026
9 checks passed
@zackkatz
zackkatz deleted the fix/mutator-move-placeholder branch July 6, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant