[CLNP-6892][fix]: Fix search result click not triggering bounce animation#1422
Conversation
✅ Deploy Preview for sendbird-uikit-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4100d6d540
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The previous React state-based animation (`animatedMessageId` → `isAnimated` → CSS class) was unreliable because scroll-related re-renders caused the useLayoutEffect to re-run and clear animation timeouts before they fired. Replace with direct DOM class manipulation that bypasses React's effect lifecycle entirely: - Apply `sendbird-msg-hoc__bounce` class directly via querySelector - Proper cleanup on rapid re-clicks (clear timers + remove class) - Scroll to message on re-click when scrolled away Fixes CLNP-6892 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4100d6d to
f5df1f9
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Uikit SDK를 제공하는 입장에서 Dom 자체를 직접 handling하는 건 지양하는 게 좋을 것 같습니다. 해서 React lifecycle을 우회하는 방식보다는, 정화한 순서에 맞게 동작하는 방식을 찾는 게 더 좋아 보입니다. |
Replace timer-based animation with CSS onAnimationEnd event and protect animation state with local showBounce state to prevent external store changes from interrupting the animation. Changes: - MessageView: use showBounce local state + onAnimationEnd instead of setTimeout timers. Remove messageScrollRef.current from deps. - GroupChannelProvider: use scrollToMessage with messageFocusAnimated for animated message effect. Skip startingPoint scroll when animatedMessageId is set to prevent conflicts. - useGroupChannel: remove setAnimatedMessageId(null) from scrollToMessage to prevent intermediate null state from canceling animation via useSyncExternalStore re-render. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When navigating from thread to parent message, DesktopLayout sets startingPoint to null before setting animatedMessageId. Fall back to setAnimatedMessageId directly when startingPoint is not available, since scroll is already handled by the earlier startingPoint effect. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e3a1356ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (_animatedMessageId) { | ||
| actions.setAnimatedMessageId(_animatedMessageId); | ||
| if (typeof startingPoint === 'number') { | ||
| // Search result click: scroll to message and animate | ||
| actions.scrollToMessage(startingPoint, _animatedMessageId, true, false); | ||
| } else { |
There was a problem hiding this comment.
Defer animated scroll until channel state is initialized
When animatedMessageId is provided on initial mount (e.g., deep-link/open-with-highlight), this effect can run before message data is initialized and call actions.scrollToMessage(...) too early. In useGroupChannel.scrollToMessage, the fallback path is gated by state.initialized, so the call becomes a no-op while uninitialized; because this effect only depends on _animatedMessageId, it does not retry after initialization, and the new _animatedMessageId guard in the starting-point effect also suppresses the old scroll path. The result is that initial highlight navigation can fail to scroll/animate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in the latest commit.
The animated-message effect now also depends on state.initialized so it retries after the channel finishes initializing. The starting-point effect's !_animatedMessageId guard still keeps the two effects mutually exclusive, so there's no double-scroll path.
useEffect(() => {
if (_animatedMessageId && state.initialized) {
if (typeof startingPoint === 'number') {
actions.scrollToMessage(startingPoint, _animatedMessageId, true, false);
} else {
actions.setAnimatedMessageId(_animatedMessageId);
}
}
}, [_animatedMessageId, state.initialized]);Thanks!
| } | ||
| }, [isAnimationTarget]); | ||
|
|
||
| const handleAnimationEnd = useCallback((e: React.AnimationEvent) => { |
There was a problem hiding this comment.
fallback으로 cleanup이 되는 부분이 필요가 없을까요? Uikit이 custom render를 제공하고 있어서, 정확한 루트로 진행이 안 될 수도 있습니다. fallback clean up이 가능하다면 같이 있으면 좋을 것 같습니다.
There was a problem hiding this comment.
말씀하신 대로 bounce keyframe 이 디센던트 .sendbird-message-content 에 걸려 있어서 custom renderMessage 결과물에 그 클래스가 없으면 onAnimationEnd 가 발화되지 않아 animation state 가 stuck 되는 회귀가 있었습니다.
최신 커밋에서 fallback setTimeout 을 추가해 CSS animation duration(1s) + 버퍼 후 강제 정리하도록 수정했습니다.
fallbackTimerRef로 timer 추적 →handleAnimationEnd가 정상 발화하면 clear 해서 이중 정리 방지- cleanup 로직은 ref 에 담아 effect deps 가
[isAnimationTarget]만 의존하도록 안정화 → 부모가onMessageAnimated를 memoize 하지 않아도 fallback timer 가 조기 clear 되지 않게 처리했습니다
감사합니다!
- GroupChannelProvider: depend on state.initialized in the animated-message effect so deep-link / direct Provider usage (animatedMessageId + startingPoint set on initial mount) retries scroll+animate after the channel finishes initializing. Without it, scrollToMessage runs while messages are empty and the starting-point effect is suppressed by the !_animatedMessageId guard, leaving the message un-scrolled. - MessageView: add a setTimeout fallback so animation cleanup still runs when consumers supply a custom renderMessage. The bounce keyframe is applied to the descendant .sendbird-message-content, which custom renders do not include, so onAnimationEnd never fires and the animation state would otherwise stay stuck. Cleanup logic is kept in a ref so the effect deps stay stable even when onMessageAnimated is not memoized. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous React state-based animation (
animatedMessageId→isAnimated→ CSS class) was unreliable because scroll-related re-renders caused the useLayoutEffect to re-run and clear animation timeouts before they fired.Replace with direct DOM class manipulation that bypasses React's effect lifecycle entirely:
sendbird-msg-hoc__bounceclass directly via querySelectorFixes CLNP-6892
For Internal Contributors
{type}/TICKET_ID/descriptionfeat/fix/chore/doc/releaseTemplate
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If unsure, ask the members.This is a reminder of what we look for before merging your code.
External Contributions
This project is not yet set up to accept pull requests from external contributors.
If you have a pull request that you believe should be accepted, please contact
the Developer Relations team developer-advocates@sendbird.com with details
and we'll evaluate if we can set up a CLA to allow for the contribution.