[CLNP-8774] fix: prevent TypingIndicator from leaking group channel handlers on unmount - #1448
Open
sf-tyler-jeong wants to merge 2 commits into
Open
[CLNP-8774] fix: prevent TypingIndicator from leaking group channel handlers on unmount#1448sf-tyler-jeong wants to merge 2 commits into
sf-tyler-jeong wants to merge 2 commits into
Conversation
…nmount The effect cleanup removed the stale `handlerId` state instead of the `newHandlerId` that was actually registered, so the live GroupChannelHandler leaked on unmount and accumulated across mount/unmount cycles. Hoist `newHandlerId` into the effect scope so the cleanup removes the exact id it registered, falling back to `handlerId` when no handler was added. Add a regression test covering unmount and channel-switch cleanup. CLNP-8774 Reported via #1444
✅ Deploy Preview for sendbird-uikit-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
danney-chun
reviewed
Jul 30, 2026
| useEffect(() => { | ||
| let newHandlerId: string | undefined; | ||
| if (sb?.groupChannel?.addGroupChannelHandler) { | ||
| sb.groupChannel.removeGroupChannelHandler(handlerId); |
Contributor
There was a problem hiding this comment.
handlerId는 더 이상 사용되지 않는 것 같습니다.
최초에 생성될 때 uuid로 생성되고, unmount 시에는 cleanup에서 생성된 ID로 handler를 remove하기 때문에 handlerId가 의미가 없어 보입니다.
Contributor
Author
There was a problem hiding this comment.
수정했습니다. 감사합니다!
danney-chun
reviewed
Jul 30, 2026
Contributor
There was a problem hiding this comment.
이 부분도 더 이상 state로 handler를 관리하는 게 의미가 없어 보입니다.
Contributor
Author
There was a problem hiding this comment.
수정했습니다. 감사합니다!
After the handler-leak fix, the handlerId React state no longer serves a purpose: the cleanup removes the exact id it registered via a local variable, so the state (and the redundant remove at the start of the effect body) was dead code. Keeping the id in state was in fact what made the original add/remove ids diverge and leak. Use a single local handlerId per effect run, registered and removed within the same closure -- matching the pattern already used in Thread/useHandleChannelEvents and the other channel handlers. Behavior is unchanged and the CLNP-8774 regression test still passes. Addresses review feedback on #1448. CLNP-8774
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
TypingIndicator'suseEffectcleanup removed the stalehandlerIdstateinstead of the
newHandlerIdthat was actually registered viaaddGroupChannelHandler. The liveGroupChannelHandlerwas therefore neverremoved on unmount and leaked, accumulating across mount/unmount cycles (e.g.
repeated channel navigation). Originally reported in #1444.
Changes
newHandlerIdinto the effect scope so the cleanup removes the exactid it registered, falling back to
handlerIdwhen no handler was added.the previous code, passes with the fix).
Channel/components/TypingIndicatorre-exports this component, so bothmodule entry points are covered by the one change.