Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The core regression path lacks automated test coverage.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes editor carousel selection by mapping slide indexes to their containing Embla scroll snaps.
Changes:
- Uses Embla’s slide registry to resolve the correct snap.
- Prevents grouped-slide selections from scrolling out of view.
| File | Description |
|---|---|
src/blocks/carousel/viewport/edit.tsx |
Maps selected slides to snap indexes before scrolling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const snapIndex = api | ||
| .internalEngine() | ||
| .slideRegistry.findIndex( ( slides ) => slides.includes( selectedSlideIndex ) ); |
There was a problem hiding this comment.
the useSelect mock always returns no selected block, so the effect exits before reaching the snap mapping.
The real library runs in jsdom, where every size is 0, so it can't produce a grouped registry like [[0, 1], [2, 3]]. Covering this needs a new jest.mock( 'embla-carousel' ) with a stubbed internalEngine().slideRegistry, plus a selection mock that can be set per test.
Summary
Selecting a slide (or any block inside one, including from List View) in the editor scrolled the carousel to a different slide whenever Slides to scroll was greater than 1, pushing the selected block out of view. The selection-sync effect passed a slide index to Embla's
scrollTo()/selectedScrollSnap(), which work in scroll snap indexes. This maps the slide to the snap that contains it before scrolling.Type of change
Related issue(s)
Closes #201
What changed
viewport/edit.tsx: map the selected slide index to its scroll snap through Embla'sinternalEngine().slideRegistry(respectsslidesToScroll,containScrolland loop) before callingselectedScrollSnap()/scrollTo(). This is the same lookup Embla uses internally for slide focus.save()output, block attributes and the frontendview.jsbundle are unchanged (verified by comparing build output with and without the change), so no deprecation is needed.Breaking changes
Does this introduce a breaking change? If yes, describe the impact and migration path below.
Testing
Describe how this was tested.
Test details:
slidesToScroll1–3, loop on/off,trimSnaps/keepSnapsand start/center alignment. WithslidesToScroll: 1the scroll target is identical in every case. Every case where it differs (slidesToScroll> 1) was one where the old target didn't contain the selected slide.Screenshots / recordings
Screen.Recording.2026-10-06.at.5.09.09.PM.mov
Checklist