Repository navigation
feat: CW0 pilot — port crosswinds-blocks/breadcrumbs as artisanpack/breadcrumbs - #567
Merged
ViewFromTheBox merged 2 commits intoJun 11, 2026
Conversation
…readcrumbs Forks the upstream crosswinds-blocks breadcrumbs block into the visual-editor package and establishes the pattern for CW1–CW7 to follow: - Editor scaffold under `resources/js/visual-editor/blocks/breadcrumbs/` (block.json, edit, save, index, inserter-icon, separators, css). Drops the upstream `breadrcumbsSchema` typo for `breadcrumbsSchema` since the fresh namespace has no legacy markup to preserve. - Blade + React + Vue renderers reading a server-stamped `_resolvedTrail` attribute, emitting `<nav><ol>…</ol></nav>` with the selected chevron separator and optional schema.org BreadcrumbList microdata. URLs run through the existing UrlSanitizer / safeUrl helpers so unsafe schemes are dropped. - Three-renderer parity tests (12 new assertions per renderer) covering schema-on/schema-off, separator fallback, empty trail, and unsafe-URL rejection. Manifest in `packages/renderer-parity.json` updated. No transforms — upstream did not define any for breadcrumbs. Follow-ups filed under umbrella #495: - #565 — BreadcrumbsResolver to stamp `_resolvedTrail` on the server - #566 — Editor canvas iframe pipeline for custom block CSS Closes #496
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds an artisanpack/breadcrumbs block: block metadata and editor UI, shared separator model, Blade/React/Vue renderers that normalize/sanitize _resolvedTrail and optionally emit Schema.org microdata, registry wiring, styles, and parity tests across all renderers. ChangesBreadcrumbs Block Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/visual-editor-renderer-react/src/blocks/artisanpack/breadcrumbs.tsx (1)
114-114: ⚡ Quick winConsider using index-only React key for stability.
Currently the key combines
positionanditem.label. While position ensures uniqueness, using the loop index directly would be simpler and more stable. Labels might contain special characters or be duplicated in edge cases.♻️ Suggested improvement
Replace the existing map callback to capture the index parameter and use it in the key:
- {trail.map((item, index) => { + {trail.map((item, idx) => { const isLast = index === trail.length - 1; const position = index + 1; const itemProps = breadcrumbsSchema ? { className: 'ap-breadcrumbs__item', itemProp: 'itemListElement', itemScope: true, itemType: 'https://schema.org/ListItem', } : { className: 'ap-breadcrumbs__item' }; return ( - <li key={`${position}-${item.label}`} {...itemProps}> + <li key={idx} {...itemProps}>Or keep
indexas the parameter name and use it directly:return ( - <li key={`${position}-${item.label}`} {...itemProps}> + <li key={index} {...itemProps}>🤖 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 `@packages/visual-editor-renderer-react/src/blocks/artisanpack/breadcrumbs.tsx` at line 114, The list item key currently uses a combination of position and item.label which can be unstable; update the map callback that renders the <li> (where variables item, position, and itemProps are used) to capture the array index (e.g., add an index parameter to the map) and use that index as the key instead of `${position}-${item.label}` so each <li> uses the loop index for stable uniqueness.
🤖 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 `@packages/visual-editor-renderer-vue/src/blocks/artisanpack/breadcrumbs.ts`:
- Around line 119-145: The else branch in artisanpack/breadcrumbs.ts always
assigns class 'ap-breadcrumbs__current' to non-link spans even when item.current
is false (happens when URL was sanitized to null); update the span rendering so
the class is added only when item.current is true (mirror the conditional used
for href branch), i.e., modify the code paths that build labelAttrs in the
non-link branch (the logic around labelAttrs and item.current) to conditionally
add 'ap-breadcrumbs__current' only when item.current === true; then apply the
same conditional class fix to the Blade and React breadcrumb renderers to keep
outputs consistent.
---
Nitpick comments:
In
`@packages/visual-editor-renderer-react/src/blocks/artisanpack/breadcrumbs.tsx`:
- Line 114: The list item key currently uses a combination of position and
item.label which can be unstable; update the map callback that renders the <li>
(where variables item, position, and itemProps are used) to capture the array
index (e.g., add an index parameter to the map) and use that index as the key
instead of `${position}-${item.label}` so each <li> uses the loop index for
stable uniqueness.
🪄 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: 1c468976-8845-4003-89b6-9e0f03b68101
📒 Files selected for processing (16)
packages/renderer-parity.jsonpackages/visual-editor-renderer-blade/resources/views/blocks/artisanpack/breadcrumbs.blade.phppackages/visual-editor-renderer-blade/tests/Unit/BlockRendererTest.phppackages/visual-editor-renderer-react/src/blocks/artisanpack/breadcrumbs.tsxpackages/visual-editor-renderer-react/src/blocks/registerCoreBlocks.tspackages/visual-editor-renderer-react/tests/BlockTree.test.tsxpackages/visual-editor-renderer-vue/src/blocks/artisanpack/breadcrumbs.tspackages/visual-editor-renderer-vue/src/blocks/registerCoreBlocks.tspackages/visual-editor-renderer-vue/tests/BlockTree.test.tsresources/js/visual-editor/blocks/breadcrumbs/block.jsonresources/js/visual-editor/blocks/breadcrumbs/breadcrumbs.cssresources/js/visual-editor/blocks/breadcrumbs/edit.tsxresources/js/visual-editor/blocks/breadcrumbs/index.tsresources/js/visual-editor/blocks/breadcrumbs/inserter-icon.tsxresources/js/visual-editor/blocks/breadcrumbs/save.tsxresources/js/visual-editor/blocks/breadcrumbs/separators.tsx
- Only mark the breadcrumb span with `ap-breadcrumbs__current` when the trail entry is actually `current`. Previously every non-link branch carried the class — so an entry whose URL was sanitized away rendered as 'current' even when it wasn't the trail's final crumb. Mirrored across all three renderers (Blade, React, Vue). - React: use the loop index for the `<li>` key instead of a position+label composite — labels can collide and the array index is already stable for this list. - Add an assertion to the unsafe-URL parity tests confirming exactly one `ap-breadcrumbs__current` occurrence in the rendered output.
ViewFromTheBox
merged commit Jun 11, 2026
fb258ee
into
feature/495-port-crosswinds-blocks
1 check passed
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
This was referenced Jun 12, 2026
19 of 22 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
Forks the upstream
crosswinds-blocks/breadcrumbsblock into the visual-editor package asartisanpack/breadcrumbsand establishes the pattern for CW1–CW7 to follow.Closes: #496
Type of Change
Related Issue
Issue: #496 (under umbrella #495)
Motivation and Context
CW0 — first of the 8-phase crosswinds-blocks port. Breadcrumbs is the cleanest pilot: single block, server-rendered, no inner blocks, no JS interactivity. Locking in the editor scaffold + three-renderer parity + tests pattern here lets CW1–CW7 fan out in parallel.
Changes Made
resources/js/visual-editor/blocks/breadcrumbs/—block.json(separatorIcon, breadcrumbsSchema; full block-supports inheriting upstream),edit.tsxwith InspectorControls panel + stub trail preview,save.tsxreturningnull(dynamic),index.ts,inserter-icon.tsx, sharedseparators.tsx,breadcrumbs.css. Dropped the upstreambreadrcumbsSchematypo forbreadcrumbsSchemasince this is a fresh namespace.packages/visual-editor-renderer-{blade,react,vue}/— read a server-stamped_resolvedTrail(Array<{ label, url?, current? }>) and emit<nav><ol>…</ol></nav>with the selected chevron separator. Schema.org BreadcrumbList microdata whenbreadcrumbsSchema=true. URLs run through the existingUrlSanitizer/safeUrlhelpers so unsafe schemes (javascript:, etc.) are dropped.packages/renderer-parity.jsonincludesartisanpack/breadcrumbs.How Has This Been Tested?
Testing Environment:
Tests Performed:
node scripts/verify-renderer-parity.mjs— passes.cr review --base feature/495-port-crosswinds-blocks) — 1 valid finding addressed (URL sanitization), 1 false-positive skipped (SVG path is correct), final pass returned 0 findings.Accessibility Tests Run
Details: The rendered
<nav>carries anaria-label(defaulting to "Breadcrumb"); the current page entry carriesaria-current="page"; separators arearia-hidden. Schema.org BreadcrumbList microdata is emitted unless the author disables it.Tests Added
Test details: 12 new renderer parity specs (4 per renderer) covering happy-path schema rendering, separator/schema fallback, unsafe-URL rejection, and empty-trail behavior.
Documentation
Documentation details: Docblocks on every new file explain the upstream provenance, the dynamic-block contract (server stamps
_resolvedTrail), and the editor-vs-renderer split. Follow-ups for runtime + iframe CSS pipeline filed as #565 and #566 under umbrella #495.Screenshots
Pre-Submission Checklist
Summary by CodeRabbit
New Features
Tests