Skip to content

fix(useRefEffect): use the updated callback when dependencies change - #483

Merged
hyesungoh merged 2 commits into
toss:mainfrom
chungyo:fix/use-ref-effect-callback
Sep 9, 2026
Merged

hyesungoh merged 2 commits into
toss:mainfrom
chungyo:fix/use-ref-effect-callback

Conversation

@chungyo

@chungyo chungyo commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Overview

When useRefEffect is attached to a DOM element and its dependencies change, React calls the ref before passive effects run. The ref previously read through usePreservedCallback, whose callback was still from the previous render. For example, changing a dependency from A to B ran cleanup(A) followed by setup(A) instead of setup(B).

This also affects useIntersectionObserver: changing the root, root margin, or threshold creates a new observer, but the ref can reattach the element to the previous observer.

Changes

  • Create a callback holder for each dependency change so the updated callback is available during ref attachment.
  • Keep effect-based callback updates when dependencies are unchanged, preserving the existing behavior when a DOM node is replaced after a later render.
  • Leave usePreservedCallback unchanged and avoid mutating an existing callback holder during render.
  • Add DOM lifecycle and observer regression tests, plus a patch changeset.

Validation

  • Confirmed five regression cases fail before the fix: dependency updates with and without StrictMode, and three observer option changes.
  • Added eight cases covering these regressions, ref stability, cleanup, later node replacement, and callbacks from a render that did not commit. The useRefEffect cases are consolidated in the existing spec.
  • Full library suite: 587 tests passed with 100% coverage on Node 24. Related useRefEffect and useIntersectionObserver tests: 21 passed in both regular and React Compiler modes.
  • Format, lint, type, docs, skill, codemod, compiler compatibility, and package validation passed locally. Lint retains existing warnings in unchanged files.
  • Verified the before/after behavior in Chromium 152 with React 18.3.1 and 19.2.4, using the native IntersectionObserver API in a separate fixture.

The public signature is unchanged. No new dependency is added.

Checklist

  • Added regression tests and confirmed the original failures.
  • Formatted and linted the changed files; full format and lint checks passed.
  • Ran yarn test:coverage with 100% coverage.
  • Reviewed the existing JSDoc; the documented signature and dependency contract are unchanged.

Copilot AI lite review requested due to automatic review settings September 9, 2026 01:19
@changeset-bot

changeset-bot Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8828030

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-simplikit Patch

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

@codecov-commenter

codecov-commenter commented Sep 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (163ac5c) to head (8828030).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #483   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        69           
  Lines         2233      2242    +9     
  Branches       718       720    +2     
=========================================
+ Hits          2233      2242    +9     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

A new test file uses *.test.tsx naming in a hook folder that already follows the older *.spec.ts convention, and it should be renamed/migrated to match repo conventions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes a subtle ref-attachment timing issue in useRefEffect where dependency changes could still invoke the previous render’s callback during DOM ref commit, which in turn impacted downstream hooks like useIntersectionObserver.

Changes:

  • Reworked useRefEffect to use a dependency-scoped callback holder so the updated callback is available during ref attachment (before passive effects run).
  • Added DOM lifecycle regression tests for useRefEffect and an observer migration regression test for useIntersectionObserver.
  • Added a patch changeset documenting the bugfix.
File summaries
File Description
packages/react-simplikit/src/hooks/useRefEffect/useRefEffect.ts Creates a new callback holder when deps change and keeps committed callback updates via useEffect to avoid stale callback reads during ref attachment.
packages/react-simplikit/src/hooks/useRefEffect/useRefEffect.test.tsx Adds DOM/ref lifecycle regression coverage for dependency changes, StrictMode behavior, conditional attachment, and Suspense.
packages/react-simplikit/src/hooks/useIntersectionObserver/useIntersectionObserver.spec.ts Adds regression asserting a mounted element is moved to the newly created observer when observer options change.
.changeset/fix-ref-effect-callback.md Patch changeset describing the behavioral fix for dependency changes during ref attachment.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@hyesungoh hyesungoh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for contribution! 👍 👍

@hyesungoh
hyesungoh merged commit 6605918 into toss:main Sep 9, 2026
19 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 9, 2026
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.

4 participants