refactor: move onboarding animation state into event handlers#1024
refactor: move onboarding animation state into event handlers#1024qorexdev wants to merge 2 commits intoMODSetter:devfrom
Conversation
Remove useEffect that watched stepIndex to drive shouldAnimate/contentKey. stepIndex only changes from handleNext/handlePrev, so set shouldAnimate directly in those handlers. Replace contentKey state (always equal to stepIndex) with stepIndex as the animation key. Fixes MODSetter#1017
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 947def5..3b33a3e
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (1)
• surfsense_web/components/onboarding-tour.tsx
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
The refactor cleanly removes the useEffect/useRef pattern in TourTooltip by placing setShouldAnimate(true) at the call sites in handleNext and handlePrev — this makes the causality explicit and easier to follow. However, if any other navigation paths exist in OnboardingTour (e.g., keyboard shortcuts or a direct step-jump handler not shown in this diff), they would silently skip the animation since setShouldAnimate(true) would need to be added there too; the old useEffect approach was immune to this since it reacted to any stepIndex change regardless of source.
The onAnimationEnd prop is passed as an inline arrow function (() => setShouldAnimate(false)) at line 769, which creates a new function reference on every render of OnboardingTour. This is harmless today, but if TourTooltip is ever wrapped in React.memo for performance, it will defeat memoization — wrapping it in useCallback alongside handleNext/handlePrev would be more consistent with the existing pattern in the file.
|
@qorexdev is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
qorexdev
left a comment
qorexdev
left a comment
There was a problem hiding this comment.
Good catches, thanks for the review.
On the keyboard/direct-jump concern: Looking at the current implementation, the only keyboard handler in OnboardingTour is the Escape key which calls setIsActive(false) — it doesn't navigate between steps. So there's no silent path today, but you're right that if arrow-key navigation or a step-jump handler is added later, it would need an explicit setShouldAnimate(true). Worth keeping in mind.
On useCallback for onAnimationEnd: Valid — it was inconsistent with the pattern used for handleNext, handlePrev, etc. Extracted it into handleAnimationEnd = useCallback(() => setShouldAnimate(false), []) in the latest commit.
2 participants
Summary
Fixes #1017.
shouldAnimateandcontentKeyinTourTooltipwere driven by auseEffectwatchingstepIndex. SincestepIndexonly ever changes from user-triggeredhandleNext/handlePrev, the effect is unnecessary.Changes
useEffect(lines 180–188) that setshouldAnimate/contentKeyonstepIndexchangecontentKeystate andprevStepIndexRef— both were always equal tostepIndex; replacedkey={contentKey}withkey={stepIndex}directlyshouldAnimatestate to the parentOnboardingTour; setsetShouldAnimate(true)insidehandleNextandhandlePrevwhen the step actually changesshouldAnimateandonAnimationEndas props toTourTooltipAcceptance criteria checklist
useEffectfor animation state inTourTooltipcontentKeystate variable removed;stepIndexused directly as the animation keyonAnimationEndcallback still resetsshouldAnimatetofalseHigh-level PR Summary
This PR refactors the onboarding tour animation logic by eliminating an unnecessary
useEffectthat was watchingstepIndexchanges. The animation state (shouldAnimate) is now lifted to the parentOnboardingTourcomponent and set directly within thehandleNextandhandlePrevevent handlers. Additionally, the redundantcontentKeystate variable andprevStepIndexRefare removed since they were always equal tostepIndex, which is now used directly as the animation key. This simplifies the component structure while maintaining the same fade-in slide animation behavior.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_web/components/onboarding-tour.tsx