fix(core): Change useMutationObserver to useMutationObserverRef #332
Conversation
Replace the useEffect-based useMutationObserver hook with a callback ref implementation (useMutationObserverRef) to improve performance and resolve dependency issues. Key improvements: - Use callback refs instead of useEffect to avoid stale dependency issues - Store callback and options in refs for React 17+ compatibility - Empty dependency array ensures stable ref callback across renders - Eliminate unnecessary observer reconnections on re-renders - Add cleanup function return for React 19+ support Updated components: - Tooltip: Use new useMutationObserverRef hook - Popover: Use new useMutationObserverRef hook - NavigationMenu: Use new useMutationObserverRef hook Testing: - Added comprehensive test suite for useMutationObserverRef (5 tests) - All 248 existing tests pass with new implementation - Added test for React 19 cleanup function support References: - https://tkdodo.eu/blog/avoiding-use-effect-with-callback-refs - https://tkdodo.eu/blog/ref-callbacks-react-19-and-the-compiler BREAKING CHANGE: Removed useMutationObserver hook in favor of useMutationObserverRef. This is an internal API change and should not affect external consumers.
SimYunSup
requested review from
MaxLee-dev,
ZeroChoi2781 and
noahchoii
as code owners
🦋 Changeset detectedLatest commit: 7259abb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@SimYunSup is attempting to deploy a commit to the goorm Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @SimYunSup, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the robustness and future-proofing of DOM mutation observation within the UI components. It involves a strategic refactoring that replaces an existing hook with a new, more advanced implementation leveraging React's callback refs. This change is designed to provide a more stable and compatible solution for tracking DOM changes, especially in anticipation of upcoming React versions, while also ensuring that the components relying on this functionality benefit from increased reliability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
useMutationObserver 훅을 callback ref를 사용하는 useMutationObserverRef로 리팩토링하여 안정성과 React 19+ 호환성을 개선한 점이 좋습니다. 새로운 훅의 구현 방식은 최신 React 패턴을 잘 따르고 있으며, 관련 테스트 케이스도 포괄적으로 추가되었습니다.
리뷰 과정에서 새로운 훅의 정리(cleanup) 함수에서 발생할 수 있는 미묘한 경쟁 상태(race condition)와, 이를 검증하는 테스트 케이스의 논리적 결함을 발견했습니다. 코드의 안정성을 더욱 높이기 위해 이 두 가지 사항에 대한 수정 의견을 댓글로 남겼으니 확인 부탁드립니다. 전반적으로 훌륭한 리팩토링입니다.
| it('should return cleanup function that disconnects observer (React 19+ support)', async () => { | ||
| const callback = vi.fn(); | ||
| let cleanupFn: (() => void) | undefined; | ||
|
|
||
| function TestComponent() { | ||
| const ref = useMutationObserverRef<HTMLDivElement>({ | ||
| callback, | ||
| options: { attributes: true }, | ||
| }); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={(node) => { | ||
| const cleanup = ref(node); | ||
| // Store cleanup function if returned (React 19+) | ||
| if (typeof cleanup === 'function') { | ||
| cleanupFn = cleanup; | ||
| } | ||
| }} | ||
| data-test="initial" | ||
| > | ||
| Test Content | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const { unmount } = render(<TestComponent />); | ||
|
|
||
| // If cleanup function was returned, it should work | ||
| if (cleanupFn) { | ||
| cleanupFn(); | ||
| expect(callback).not.toHaveBeenCalled(); | ||
| } | ||
|
|
||
| unmount(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
should return cleanup function that disconnects observer (React 19+ support) 테스트 케이스가 정리 함수의 동작을 올바르게 검증하지 못하고 있습니다.
현재 테스트는 cleanupFn()을 호출한 직후 callback이 호출되지 않았는지만 확인합니다. 하지만 이 시점에는 어떠한 DOM 변경도 발생하지 않았기 때문에 callback은 원래 호출되지 않는 것이 정상입니다.
정리 함수가 observer를 제대로 disconnect하는지 확인하려면, cleanupFn() 호출 이후에 DOM 변경을 발생시키고, 그 결과로 callback이 호출되지 않는 것을 확인해야 합니다.
아래와 같이 테스트를 수정하여 정리 함수의 동작을 더 정확하게 검증하는 것을 제안합니다.
it('should return cleanup function that disconnects observer (React 19+ support)', async () => {
const callback = vi.fn();
let cleanupFn: (() => void) | undefined;
let testRef: HTMLDivElement | null = null;
function TestComponent() {
const ref = useMutationObserverRef<HTMLDivElement>({
callback,
options: { attributes: true, attributeFilter: ['data-test'] },
});
return (
<div
ref={(node) => {
testRef = node;
const cleanup = ref(node);
// React 19+에서는 정리 함수가 반환될 수 있습니다.
if (typeof cleanup === 'function') {
cleanupFn = cleanup;
}
}}
data-test="initial"
>
Test Content
</div>
);
}
const { unmount } = render(<TestComponent />);
await waitFor(() => expect(testRef).not.toBeNull());
// 정리 함수가 반환되었다면 (React 19+), 호출하여 observer 연결을 끊습니다.
if (cleanupFn) {
cleanupFn();
// 정리 함수 호출 후에 DOM 변경을 트리거합니다.
testRef?.setAttribute('data-test', 'changed-after-cleanup');
// 비동기 콜백이 실행될 수 있는 시간을 잠시 줍니다.
await new Promise((resolve) => setTimeout(resolve, 50));
// observer 연결이 끊겼으므로 콜백은 호출되지 않아야 합니다.
expect(callback).not.toHaveBeenCalled();
}
unmount();
});
noahchoii
added
scope: navigation-menu
Fix TypeScript type narrowing issues in test file by using non-null assertion operator after waitFor checks confirm testRef is not null. Changes: - Use non-null assertion (!) for testRef after null checks - Remove unnecessary eslint-disable comments - All 248 tests pass - TypeScript typecheck passes without errors
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
3 participants
Related Issues
No related Issues
Description of Changes
This pull request refactors how mutation observers are attached to DOM elements in several UI components. The main change is replacing the custom
useMutationObserverhook (which used auseRefanduseEffect) with a newuseMutationObserverRefhook that uses callback refs for improved reliability and compatibility with React 19+. Comprehensive tests for the new hook have also been added.Checklist
Before submitting the PR, please make sure you have checked all of the following items.