Added ... Copy/Paste of Blocks, Collapsible Sections, Shared Field Includes, Recent Blocks

[helmutkaufmann]

Summary by CodeRabbit

• New Features

  • Collapsible sections in block forms with persisted open/closed state
  • Block definition includes to share common fields across blocks
  • Recently used blocks pinned in the Add Block palette
  • Copy, cut, paste, and duplicate actions for managing blocks

• Documentation

  • Expanded documentation covering new block definition features and block management capabilities

[coderabbitai]

Walkthrough

This PR introduces three major features for the Blocks plugin (v1.1.0). BlockManager gains YAML include: directive support: block definition files can reference shared YAML files whose fields and config sections are recursively merged, with circular-include guarding and type-collision warnings. Blocks.php adds a collapsible: true shorthand for type: section fields, translating it to data-block-collapsible container attributes at build time. The block item partial is redesigned around a unified horizontal toolbar, and a large inline JavaScript block bootstraps client-side features: clipboard copy/cut/paste/duplicate using sessionStorage, collapsible section state persistence via localStorage, and recently-used block pinning in the Add Block palette. Tests, fixtures, documentation, and version metadata are updated accordingly.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 79.17% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes all four major features introduced in the PR: Copy/Paste of Blocks, Collapsible Sections, Shared Field Includes, and Recent Blocks, matching the implementation scope.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

tests/classes/BlockManagerTest.php (1)

152-167: ⚡ Quick win

Add a collision-order assertion in testMultipleIncludes().

This test currently validates only key presence. It should also assert that for the same field key across multiple includes, the later include wins, so precedence regressions are caught.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/classes/BlockManagerTest.php` around lines 152 - 167, The
testMultipleIncludes() method currently only validates that expected fields
exist via assertArrayHasKey calls, but does not verify collision-order behavior
when the same field key appears in multiple includes. Add an assertion that
confirms when a field key exists in both _base.yaml and _shared.yaml includes,
the later include (_shared.yaml) takes precedence by checking the actual field
properties (such as label or type) match the expected value from the later
include, not the earlier one. This ensures field precedence order is maintained
and regressions in override behavior are caught.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@classes/BlockManager.php`:
- Around line 157-196: The issue is in the nested loop structure where
$config[$key] is progressively modified across multiple path iterations, causing
earlier merged includes to be treated as block-owned values on subsequent
iterations. This breaks the intended precedence where later includes should
override earlier includes, and block values should override all includes. To fix
this, extract and preserve the original block's own values (the $key entries
from the initial $config) before the outer path loop starts, then use this
preserved original value as $own in each iteration instead of reusing the
progressively merged $config[$key]. This ensures each include is merged against
the block's true original values, not against accumulated results from previous
include merges.

In `@formwidgets/blocks/assets/less/blocks.less`:
- Around line 66-68: Replace all occurrences of the `.transform()` mixin with
the standard CSS `transform` property to fix the stylelint error. Specifically,
change `.transform(rotate(180deg));` to the standard CSS syntax `transform:
rotate(180deg);` in the selector `.field-block-item.collapsed >
.repeater-item-remove .repeater-item-collapse-one` and any other locations where
the `.transform()` mixin is used in the stylesheet.

In `@formwidgets/blocks/partials/_block.php`:
- Around line 175-205: The serializeBlockItem function extracts only the
trailing [key] segment from input names using the regex match, which loses the
full qualified name context and causes collisions when different fields share
the same trailing key. Additionally, both serializeBlockItem and
fillFromClipboard only preserve the .value property, failing to capture
checkbox/radio checked states and other input semantics. To fix this, preserve
the complete input name (or a fully qualified identifier) instead of just the
trailing [key] segment, and when serializing and restoring field data in
serializeBlockItem and fillFromClipboard, capture and restore all relevant input
properties including .value, .checked state, .selected state, and any other
attributes necessary to fully represent each input's state.

In `@updates/version.yaml`:
- Line 5: Remove the line containing the tabs and secondaryTabs reference from
the version notes. This entry advertises support that was removed and never
rendered properly, so leaving it in the 1.1.0 release notes would mislead users
who are upgrading from older block definitions. Simply delete the entire line
that mentions 'Tabs and secondaryTabs support in block definitions'.

---

Nitpick comments:
In `@tests/classes/BlockManagerTest.php`:
- Around line 152-167: The testMultipleIncludes() method currently only
validates that expected fields exist via assertArrayHasKey calls, but does not
verify collision-order behavior when the same field key appears in multiple
includes. Add an assertion that confirms when a field key exists in both
_base.yaml and _shared.yaml includes, the later include (_shared.yaml) takes
precedence by checking the actual field properties (such as label or type) match
the expected value from the later include, not the earlier one. This ensures
field precedence order is maintained and regressions in override behavior are
caught.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9d5b25b6-a547-4e20-a81d-a02c8dbe50e1

📥 Commits

Reviewing files that changed from the base of the PR and between 6a824f9 and d2c385a.

📒 Files selected for processing (18)

• .gitignore
• CHANGELOG.md
• README.md
• classes/BlockManager.php
• composer.json
• formwidgets/Blocks.php
• formwidgets/blocks/assets/less/blocks.less
• formwidgets/blocks/partials/_block.php
• formwidgets/blocks/partials/_block_item.php
• tests/classes/BlockManagerTest.php
• tests/fixtures/blocks/includes/_base.yaml
• tests/fixtures/blocks/includes/_cycle_a.yaml
• tests/fixtures/blocks/includes/_cycle_b.yaml
• tests/fixtures/blocks/includes/_shared.yaml
• tests/fixtures/blocks/includes/_with_nested.yaml
• tests/formwidgets/BlocksTest.php
• updates/version.yaml
• winter.mix.js

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix include-order precedence across multiple includes.

The current loop reuses progressively merged $config[$key] as $own, so earlier include values are treated like block-owned values on later iterations. In collision cases, this makes earlier includes win over later ones.

Suggested fix (preserve: later include overrides earlier include; block overrides all includes)

-        $mergeKeys = ['fields', 'config'];
+        $mergeKeys = ['fields', 'config'];
+        $ownByKey = [];
+        $mergedIncludes = [];
+        foreach ($mergeKeys as $key) {
+            $ownByKey[$key] = (isset($config[$key]) && is_array($config[$key])) ? $config[$key] : [];
+            $mergedIncludes[$key] = [];
+        }

         foreach ($paths as $path) {
             if (!is_string($path) || $path === '') {
                 continue;
             }
@@
             foreach ($mergeKeys as $key) {
                 if (!isset($included[$key]) || !is_array($included[$key])) {
                     continue;
                 }

-                $own = (isset($config[$key]) && is_array($config[$key])) ? $config[$key] : [];
-
                 // Warn when a field is redefined with a different type.
-                $this->warnOnTypeCollisions($key, $included[$key], $own);
+                $this->warnOnTypeCollisions($key, $included[$key], $ownByKey[$key]);

-                // Included definitions form the base; the block's own win on collision.
-                $config[$key] = array_replace_recursive($included[$key], $own);
+                // Merge includes in order (later include wins over earlier include).
+                $mergedIncludes[$key] = array_replace_recursive($mergedIncludes[$key], $included[$key]);
             }
         }

+        // Block-owned definitions win over merged includes.
+        foreach ($mergeKeys as $key) {
+            if (!empty($mergedIncludes[$key]) || !empty($ownByKey[$key])) {
+                $config[$key] = array_replace_recursive($mergedIncludes[$key], $ownByKey[$key]);
+            }
+        }
+
         return $config;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	$mergeKeys = ['fields', 'config'];
	foreach ($paths as $path) {
	if (!is_string($path) || $path === '') {
	continue;
	}
	$realPath = File::symbolizePath($path);
	if (!$realPath || !File::exists($realPath)) {
	Log::warning("Winter.Blocks: included file not found: {$path}");
	continue;
	}
	$canonical = PathResolver::standardize($realPath);
	if (in_array($canonical, $visited, true)) {
	Log::warning("Winter.Blocks: circular include detected, skipping: {$path}");
	continue;
	}
	$included = Yaml::parse(File::get($realPath));
	if (!is_array($included)) {
	continue;
	}
	// Resolve nested includes first so they form the deepest base layer.
	$included = $this->resolveIncludes($included, array_merge($visited, [$canonical]));
	foreach ($mergeKeys as $key) {
	if (!isset($included[$key]) || !is_array($included[$key])) {
	continue;
	}
	$own = (isset($config[$key]) && is_array($config[$key])) ? $config[$key] : [];
	// Warn when a field is redefined with a different type.
	$this->warnOnTypeCollisions($key, $included[$key], $own);
	// Included definitions form the base; the block's own win on collision.
	$config[$key] = array_replace_recursive($included[$key], $own);
	}
	$mergeKeys = ['fields', 'config'];
	$ownByKey = [];
	$mergedIncludes = [];
	foreach ($mergeKeys as $key) {
	$ownByKey[$key] = (isset($config[$key]) && is_array($config[$key])) ? $config[$key] : [];
	$mergedIncludes[$key] = [];
	}
	foreach ($paths as $path) {
	if (!is_string($path) || $path === '') {
	continue;
	}
	$realPath = File::symbolizePath($path);
	if (!$realPath || !File::exists($realPath)) {
	Log::warning("Winter.Blocks: included file not found: {$path}");
	continue;
	}
	$canonical = PathResolver::standardize($realPath);
	if (in_array($canonical, $visited, true)) {
	Log::warning("Winter.Blocks: circular include detected, skipping: {$path}");
	continue;
	}
	$included = Yaml::parse(File::get($realPath));
	if (!is_array($included)) {
	continue;
	}
	// Resolve nested includes first so they form the deepest base layer.
	$included = $this->resolveIncludes($included, array_merge($visited, [$canonical]));
	foreach ($mergeKeys as $key) {
	if (!isset($included[$key]) || !is_array($included[$key])) {
	continue;
	}
	// Warn when a field is redefined with a different type.
	$this->warnOnTypeCollisions($key, $included[$key], $ownByKey[$key]);
	// Merge includes in order (later include wins over earlier include).
	$mergedIncludes[$key] = array_replace_recursive($mergedIncludes[$key], $included[$key]);
	}
	}
	// Block-owned definitions win over merged includes.
	foreach ($mergeKeys as $key) {
	if (!empty($mergedIncludes[$key]) || !empty($ownByKey[$key])) {
	$config[$key] = array_replace_recursive($mergedIncludes[$key], $ownByKey[$key]);
	}
	}
	return $config;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@classes/BlockManager.php` around lines 157 - 196, The issue is in the nested
loop structure where $config[$key] is progressively modified across multiple
path iterations, causing earlier merged includes to be treated as block-owned
values on subsequent iterations. This breaks the intended precedence where later
includes should override earlier includes, and block values should override all
includes. To fix this, extract and preserve the original block's own values (the
$key entries from the initial $config) before the outer path loop starts, then
use this preserved original value as $own in each iteration instead of reusing
the progressively merged $config[$key]. This ensures each include is merged
against the block's true original values, not against accumulated results from
previous include merges.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the stylesheet parses cleanly after the change
npx stylelint formwidgets/blocks/assets/less/blocks.less --formatter verbose

Repository: wintercms/wn-blocks-plugin

Length of output: 318

---

🏁 Script executed:

# First, let's read the blocks.less file to understand the context around lines 66-68
cat -n formwidgets/blocks/assets/less/blocks.less | sed -n '60,75p'

Repository: wintercms/wn-blocks-plugin

Length of output: 686

---

🏁 Script executed:

# Search for .transform mixin definitions in the repository
rg '\.transform\s*\(' --type less --type css

Repository: wintercms/wn-blocks-plugin

Length of output: 240

---

🏁 Script executed:

# Check if there are any LESS imports or mixins that might define .transform
rg 'transform' formwidgets/blocks/assets/less/ --type less -A 2 -B 2 | head -50

Repository: wintercms/wn-blocks-plugin

Length of output: 3124

---

🏁 Script executed:

# Search for .transform and .transition mixin definitions in LESS files
find . -name '*.less' -type f | xargs rg -l 'transform\s*\(' | head -10

Repository: wintercms/wn-blocks-plugin

Length of output: 115

---

🏁 Script executed:

# Check if there's a common mixins file that might define .transform
find . -name '*mixin*' -o -name '*variable*' | grep -E '\.(less|css)$'

Repository: wintercms/wn-blocks-plugin

Length of output: 52

---

🏁 Script executed:

# Check the LESS compilation/import chain for this file
head -50 formwidgets/blocks/assets/less/blocks.less

Repository: wintercms/wn-blocks-plugin

Length of output: 1546

---

🏁 Script executed:

# Check the imported boot.less file for mixin definitions
cat modules/backend/assets/less/core/boot.less | head -100

Repository: wintercms/wn-blocks-plugin

Length of output: 145

---

🏁 Script executed:

# Search for .transform mixin definition in the backend assets
find modules/backend -name '*.less' -type f | xargs rg '\.transform\s*\(' -B 2 -A 2 | head -40

Repository: wintercms/wn-blocks-plugin

Length of output: 945

---

🏁 Script executed:

# Check if .transform is defined as a LESS mixin
rg '`@mixin`|\.transform\s*\(' modules/backend/assets/less/ --type less -B 1 -A 3

Repository: wintercms/wn-blocks-plugin

Length of output: 139

---

🏁 Script executed:

# List the directory structure to understand the import path
ls -la | head -20

Repository: wintercms/wn-blocks-plugin

Length of output: 1259

---

🏁 Script executed:

# Check if there are any LESS files that might define mixins
find . -name '*.less' -type f -exec grep -l '\.transform\|`@mixin`' {} \;

Repository: wintercms/wn-blocks-plugin

Length of output: 115

---

🏁 Script executed:

# Look for any mixin definitions with a different pattern
rg 'transform.*\{' --type less -B 2 -A 2

Repository: wintercms/wn-blocks-plugin

Length of output: 52

---

🏁 Script executed:

# Check the parent directory referenced in the import
pwd
git ls-files | grep -E '(boot|mixin|variable)' | grep less

Repository: wintercms/wn-blocks-plugin

Length of output: 89

---

🏁 Script executed:

# Check the .stylelintrc.json to understand the linting configuration
cat .stylelintrc.json

Repository: wintercms/wn-blocks-plugin

Length of output: 1430

---

🏁 Script executed:

# Look for all uses of .transition and .transform in the blocks.less file
rg '\.(transform|transition)\(' formwidgets/blocks/assets/less/blocks.less

Repository: wintercms/wn-blocks-plugin

Length of output: 387

---

🏁 Script executed:

# Check if there are other LESS files in the repo that might have definitions
find . -type f -name '*.less' -exec wc -l {} + | sort -n

Repository: wintercms/wn-blocks-plugin

Length of output: 119

---

🏁 Script executed:

# Verify the proposed fix by creating a temporary test file with the corrected syntax
cat > /tmp/test.less <<'EOF'
.field-block-item.collapsed > .repeater-item-remove .repeater-item-collapse-one {
    transform: rotate(180deg);
}
EOF

# Test it with stylelint (need to check what the config expects)
cd /home/jailuser/git && npx stylelint /tmp/test.less --formatter verbose

Repository: wintercms/wn-blocks-plugin

Length of output: 124

---

🏁 Script executed:

# Also check if there's a .transform mixin available from a dependency
cat package.json 2>/dev/null || echo "No package.json found"

Repository: wintercms/wn-blocks-plugin

Length of output: 92

---

🏁 Script executed:

# Find the exact line numbers of all .transform usages
grep -n '\.transform(' formwidgets/blocks/assets/less/blocks.less

Repository: wintercms/wn-blocks-plugin

Length of output: 161

---

🏁 Script executed:

# Also check if there are other uses of .transition that might have the same issue
grep -n '\.transition(' formwidgets/blocks/assets/less/blocks.less

Repository: wintercms/wn-blocks-plugin

Length of output: 323

---

🏁 Script executed:

# Check the context around line 136 to see if it's also affected
sed -n '130,145p' formwidgets/blocks/assets/less/blocks.less

Repository: wintercms/wn-blocks-plugin

Length of output: 571

---

🏁 Script executed:

# Run stylelint on the current file to confirm all issues
npx stylelint formwidgets/blocks/assets/less/blocks.less

Repository: wintercms/wn-blocks-plugin

Length of output: 201

---

Replace the .transform() mixin calls with standard CSS transform property to unblock stylesheet linting.

Lines 67 and 136 both use .transform(...), which stylelint flags as CssSyntaxError: Unknown word .transform. This blocks CI. The fix is to use the standard CSS property instead.

Suggested patch

 .field-block-item.collapsed > .repeater-item-remove .repeater-item-collapse-one {
-    .transform(rotate(180deg));
+    transform: rotate(180deg);
 }

 > .repeater-item-collapse {
     .repeater-item-collapse-one {
-        .transform(rotate(180deg));
+        transform: rotate(180deg);
     }
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	.field-block-item.collapsed > .repeater-item-remove .repeater-item-collapse-one {
	.transform(rotate(180deg));
	}
	.field-block-item.collapsed > .repeater-item-remove .repeater-item-collapse-one {
	transform: rotate(180deg);
	}

🧰 Tools

🪛 Stylelint (17.12.0)

[error] 67-67: Unknown word .transform (CssSyntaxError)

(CssSyntaxError)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@formwidgets/blocks/assets/less/blocks.less` around lines 66 - 68, Replace all
occurrences of the `.transform()` mixin with the standard CSS `transform`
property to fix the stylelint error. Specifically, change
`.transform(rotate(180deg));` to the standard CSS syntax `transform:
rotate(180deg);` in the selector `.field-block-item.collapsed >
.repeater-item-remove .repeater-item-collapse-one` and any other locations where
the `.transform()` mixin is used in the stylesheet.

Source: Linters/SAST tools

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Clipboard serialization is lossy and can paste incorrect data.

Line 181/Line 197 collapse names to the final bracket key, so different fields with the same trailing key overwrite each other. Also, Line 187 and Line 201 only round-trip .value, which drops checkbox/radio checked state and other typed input semantics.

Suggested direction

-function serializeBlockItem(li) {
-    var data = { group: null, fields: {} };
+function serializeBlockItem(li) {
+    var data = { group: null, fields: {} };
     li.querySelectorAll('input[name], select[name], textarea[name]').forEach(function (el) {
-        var m = el.name.match(/\[([^\[\]]+)\]$/);
-        if (!m) { return; }
-        var key = m[1];
-        if (key === '_group') {
+        var name = el.name;
+        if (!name) { return; }
+        if (/\[_group\]$/.test(name)) {
             if (!data.group) { data.group = el.value; }
         } else {
-            data.fields[key] = el.value;
+            if (!data.fields[name]) { data.fields[name] = []; }
+            data.fields[name].push({
+                type: (el.type || '').toLowerCase(),
+                value: el.value,
+                checked: !!el.checked
+            });
         }
     });
     return data;
 }
 
 function fillFromClipboard(li, fields) {
     li.querySelectorAll('input[name], select[name], textarea[name]').forEach(function (el) {
-        var m = el.name.match(/\[([^\[\]]+)\]$/);
-        if (!m) { return; }
-        var key = m[1];
-        if (Object.prototype.hasOwnProperty.call(fields, key)) {
-            el.value = fields[key];
+        var bucket = fields[el.name];
+        if (!bucket || !bucket.length) { return; }
+        var state = bucket.shift();
+        if (state.type === 'checkbox' || state.type === 'radio') {
+            el.checked = !!state.checked && el.value === state.value;
+        } else {
+            el.value = state.value;
+        }
             el.dispatchEvent(new Event('change', { bubbles: true }));
-        }
     });
     refreshWidgets(li);
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@formwidgets/blocks/partials/_block.php` around lines 175 - 205, The
serializeBlockItem function extracts only the trailing [key] segment from input
names using the regex match, which loses the full qualified name context and
causes collisions when different fields share the same trailing key.
Additionally, both serializeBlockItem and fillFromClipboard only preserve the
.value property, failing to capture checkbox/radio checked states and other
input semantics. To fix this, preserve the complete input name (or a fully
qualified identifier) instead of just the trailing [key] segment, and when
serializing and restoring field data in serializeBlockItem and
fillFromClipboard, capture and restore all relevant input properties including
.value, .checked state, .selected state, and any other attributes necessary to
fully represent each input's state.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Drop the stale tabs / secondaryTabs note.

This entry advertises support that the stack says was removed because it never rendered. Leaving it in the 1.1.0 notes will mislead anyone upgrading from older block definitions.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@updates/version.yaml` at line 5, Remove the line containing the tabs and
secondaryTabs reference from the version notes. This entry advertises support
that was removed and never rendered properly, so leaving it in the 1.1.0 release
notes would mislead users who are upgrading from older block definitions. Simply
delete the entire line that mentions 'Tabs and secondaryTabs support in block
definitions'.