fix: clamp the current slide to the slide range#3701
Conversation
📝 WalkthroughWalkthroughThe carousel's ChangesInfinite Scroll Slide Index Normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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)
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. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/block/carousel/frontend-carousel.js (1)
680-683: 💤 Low valueModulo arithmetic prevents the crash but has an edge case with the duplicate lastClone.
The modulo successfully clamps all clone indices back into [1, slideEls.length], which prevents the TypeError mentioned in issue
#3695. However, there's a subtle semantic issue:When
slidesToShowequalsslideEls.length, the init code creates a duplicate clone of the last slide (lastClonein addition to the regular clone). For example, with 3 slides:
- Combined array indices 1-3: original slides 1-3
- Indices 4-6: clones of slides 1-3
- Index 7: duplicate lastClone of slide 3
The modulo
((7-1) % 3) + 1 = 1maps this to slide 1, when semantically it represents slide 3. This could cause the wrong dot to be highlighted or other minor UI inconsistencies if the user scrolls to a position where the lastClone is nearest.That said, the fix achieves its primary objective of preventing crashes. The old logic (subtracting length once) would still leave index 7 out of range (7 - 3 = 4), so this is a clear improvement. The semantic edge case is unlikely to cause serious issues in practice, since the carousel's swap logic typically keeps users away from the lastClone position during normal scrolling.
Alternative approach for perfect semantic correctness
If the lastClone edge case becomes problematic, consider tracking which source slide each clone represents:
if ( this.infiniteScroll ) { - // Clamp clone indexes back to the original slide range. - slide = ( ( slide - 1 ) % this.slideEls.length ) + 1 + // Map clones back to their source slide + if ( slide > this.slideEls.length ) { + const cloneIndex = slide - this.slideEls.length - 1 + // clones[0..slidesToShow-1] map to slides 1..slidesToShow + // clones[slidesToShow] (lastClone) maps to slideEls.length + slide = cloneIndex < this.slidesToShow ? cloneIndex + 1 : this.slideEls.length + } }However, the current modulo approach is simpler and adequate for preventing the crash.
🤖 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 `@src/block/carousel/frontend-carousel.js` around lines 680 - 683, The modulo fix prevents out-of-range indices but mishandles the special duplicate lastClone when slidesToShow === this.slideEls.length; update the logic in the block where this.infiniteScroll is handled (the code that currently does slide = ((slide - 1) % this.slideEls.length) + 1) to special-case the duplicate lastClone: detect when the index corresponds to the extra lastClone (e.g., by checking slidesToShow === this.slideEls.length and that the computed/initial slide index is the extra clone position or by inspecting the DOM node/class/clone metadata for "lastClone") and map that case to this.slideEls.length instead of applying the modulo; otherwise keep the modulo mapping.
🤖 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.
Nitpick comments:
In `@src/block/carousel/frontend-carousel.js`:
- Around line 680-683: The modulo fix prevents out-of-range indices but
mishandles the special duplicate lastClone when slidesToShow ===
this.slideEls.length; update the logic in the block where this.infiniteScroll is
handled (the code that currently does slide = ((slide - 1) %
this.slideEls.length) + 1) to special-case the duplicate lastClone: detect when
the index corresponds to the extra lastClone (e.g., by checking slidesToShow ===
this.slideEls.length and that the computed/initial slide index is the extra
clone position or by inspecting the DOM node/class/clone metadata for
"lastClone") and map that case to this.slideEls.length instead of applying the
modulo; otherwise keep the modulo mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9be04a20-ff5c-4d09-9c2e-59750f71f62e
📒 Files selected for processing (1)
src/block/carousel/frontend-carousel.js
1 participant
fixes #3695
Summary by CodeRabbit