Skip to content

fix(slider): add async to spring animation#2978

Open
gauthier-th wants to merge 1 commit intodevelopfrom
fix/slider
Open

fix(slider): add async to spring animation#2978
gauthier-th wants to merge 1 commit intodevelopfrom
fix/slider

Conversation

@gauthier-th
Copy link
Copy Markdown
Member

@gauthier-th gauthier-th commented Apr 30, 2026

Description

Due to the update of react-spring to v10 in #2874, the buttons of the slider in the Discover page where not working anymore.
This PRs fixes this and also adds a small margin to enable/disable the buttons when the scroll is very near the beginning/end of the scrollWidth.

How Has This Been Tested?

Click on the slider buttons on the homepage

Screenshots / Logs (if applicable)

These buttons where not working anymore.
image

Checklist:

  • I have read and followed the contribution guidelines.
  • Disclosed any use of AI (see our policy)
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • Successful build pnpm build
  • Translation keys pnpm i18n:extract
  • Database migration (if required)

Summary by CodeRabbit

  • Refactor
    • Improved slider animation handling by restructuring internal scroll update logic.
    • Enhanced scroll position threshold detection with refined boundary offset calculations.

@gauthier-th gauthier-th requested a review from a team as a code owner April 30, 2026 15:26
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

📝 Walkthrough

Walkthrough

The Slider component's scroll position boundary logic now includes a margin offset that adjusts when state transitions between "end" and "middle/start" occur. The animation system was refactored to convert the slide function to async, removing the global useSpring onChange callback and instead awaiting animation completion with per-call onChange behavior.

Changes

Cohort / File(s) Summary
Slider Animation Refactor
src/components/Slider/index.tsx
Added margin offset to scroll boundary transitions, converted slide to async function with awaited setX.start(), replaced global useSpring onChange with per-call spring config onChange behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • M0NsTeRRR
  • fallenbagel

Poem

🐰 A slider hops with graceful spring,
Now waits for animation's swing,
With margins drawn and boundaries tight,
Each scroll transition feels just right,
The async dance makes rhythm bright! 🎪

🚥 Pre-merge checks | ✅ 4
✅ 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 specifically addresses the main fix applied to the slider component—adding async to the spring animation. This directly corresponds to the primary change in the pull request.
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.


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
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/components/Slider/index.tsx (1)

104-142: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reuse handleScroll() after the animation.

These branches still recompute isStart/isEnd with the old exact thresholds, so the new 5px margin is ignored immediately after a button-triggered slide. If newX lands inside that buffer, the arrow state can be wrong until the debounced onScroll catches up.

Suggested simplification
       await setX.start({
         from: { x: scrollPosition },
         to: { x: newX },
         onChange: (results) => {
           if (containerRef.current) {
             containerRef.current.scrollLeft = results.value.x;
           }
         },
         reset: true,
         config: { friction: 60, tension: 500, velocity: 20 },
       })[0];

-      if (newX === 0) {
-        setScrollPos({ isStart: true, isEnd: false });
-      } else {
-        setScrollPos({ isStart: false, isEnd: false });
-      }
+      handleScroll();
     } else if (direction === Direction.RIGHT) {
       const newX = Math.min(
         scrollPosition - scrollOffset + visibleItems * cardWidth,
         containerRef.current?.scrollWidth ?? 0 - clientWidth
       );
       await setX.start({
         from: { x: scrollPosition },
         to: { x: newX },
         onChange: (results) => {
           if (containerRef.current) {
             containerRef.current.scrollLeft = results.value.x;
           }
         },
         reset: true,
         config: { friction: 60, tension: 500, velocity: 20 },
       })[0];

-      if (newX >= (containerRef.current?.scrollWidth ?? 0) - clientWidth) {
-        setScrollPos({ isStart: false, isEnd: true });
-      } else {
-        setScrollPos({ isStart: false, isEnd: false });
-      }
+      handleScroll();
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/Slider/index.tsx` around lines 104 - 142, The branches that
call setX.start(...) currently set isStart/isEnd manually using exact thresholds
and ignore the new 5px buffer; replace the manual setScrollPos(...) logic by
invoking the existing handleScroll() after the awaited animation so the same
debounced/edge-buffer logic is reused. Specifically, in both Direction.LEFT and
Direction.RIGHT branches, after the await setX.start(...) completes (and after
containerRef.scrollLeft has been updated by the animation), call handleScroll()
instead of the current setScrollPos(...) conditional blocks so arrow state is
computed consistently (use the existing containerRef, newX and handleScroll
function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/components/Slider/index.tsx`:
- Around line 104-142: The branches that call setX.start(...) currently set
isStart/isEnd manually using exact thresholds and ignore the new 5px buffer;
replace the manual setScrollPos(...) logic by invoking the existing
handleScroll() after the awaited animation so the same debounced/edge-buffer
logic is reused. Specifically, in both Direction.LEFT and Direction.RIGHT
branches, after the await setX.start(...) completes (and after
containerRef.scrollLeft has been updated by the animation), call handleScroll()
instead of the current setScrollPos(...) conditional blocks so arrow state is
computed consistently (use the existing containerRef, newX and handleScroll
function).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3468cf77-b1f5-4f1e-a3db-4e05e14e16c7

📥 Commits

Reviewing files that changed from the base of the PR and between b32ab02 and 6788715.

📒 Files selected for processing (1)
  • src/components/Slider/index.tsx

@0xSysR3ll
Copy link
Copy Markdown
Contributor

What do you think about coderabbit's suggestion ?
#2978 (review)

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.

2 participants