Skip to content

[MIL-1547] Recognize subscription.remove() as effect cleanup#471

Open
rayhanadev wants to merge 10 commits into
mainfrom
fix/rn-subscription-remove-cleanup
Open

[MIL-1547] Recognize subscription.remove() as effect cleanup#471
rayhanadev wants to merge 10 commits into
mainfrom
fix/rn-subscription-remove-cleanup

Conversation

@rayhanadev
Copy link
Copy Markdown
Member

@rayhanadev rayhanadev commented May 25, 2026

Why?

React Native APIs such as AppState.addEventListener, Keyboard.addListener, Appearance.addChangeListener, and Dimensions.addEventListener return subscription objects that are cleaned up by calling .remove() on the returned value. The cleanup detector previously recognized global teardown calls like removeEventListener, off, unsubscribe, and abort, but it did not understand this React Native subscription-object shape.

That meant effects which correctly returned sub.remove() could still be flagged as missing cleanup. At the same time, treating every .remove() call as cleanup would be too broad because remove is common on unrelated objects. This change recognizes .remove() only when the receiver is a binding created by a subscribe-like call in the same effect.

What changed?

  • Added bound-resource cleanup detection for subscription objects returned from subscribe-like calls.
  • Kept global release method matching narrow so unrelated calls like node.remove() do not hide real missing cleanups.
  • Made the shared cleanup-return helper require explicit bound subscription names at every call site.
  • Extended bound-resource collection to nested statement blocks so conditional subscription cleanup is recognized.
  • Added regression coverage for React Native .remove() cleanup, unrelated .remove() false negatives, conditional bound cleanup, and the prefer-use-sync-external-store interaction.

Before:

useEffect(() => {
  const sub = AppState.addEventListener("change", onChange);
  return () => sub.remove();
}, []);

Could be reported as missing cleanup.

After:

useEffect(() => {
  const sub = AppState.addEventListener("change", onChange);
  return () => sub.remove();
}, []);

Is accepted because sub is known to come from a subscribe-like call.

Before:

useEffect(() => {
  window.addEventListener("resize", onResize);
  return () => node.remove();
}, []);

Could become a false negative if .remove() were treated globally.

After:

useEffect(() => {
  window.addEventListener("resize", onResize);
  return () => node.remove();
}, []);

Still reports missing listener cleanup because node is not the subscription returned by the effect.

Test plan

Users can verify correctness by running:

pnpm --filter react-doctor test -- tests/regressions/state-rules/effect-needs-cleanup.test.ts tests/regressions/state-rules/prefer-use-sync-external-store.test.ts
pnpm --filter oxlint-plugin-react-doctor typecheck

Copilot AI review requested due to automatic review settings May 25, 2026 03:14
@reactreview
Copy link
Copy Markdown

reactreview Bot commented May 25, 2026

No new issues

Reviewed by reactreview for commit a7c338d. Configure here.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the effect-needs-cleanup detection heuristics to treat React Native EventSubscription cleanup (sub.remove()) as a valid effect teardown, and adds a regression test to prevent reintroducing the false positive.

Changes:

  • Add "remove" to the recognized unsubscription/cleanup method names.
  • Add a regression test covering AppState.addEventListener(...); return () => sub.remove();.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
packages/react-doctor/tests/regressions/state-rules/effect-needs-cleanup.test.ts Adds a regression test ensuring RN subscription .remove() cleanup is not flagged.
packages/oxlint-plugin-react-doctor/src/plugin/constants/react.ts Expands the unsubscription method allowlist to include remove for RN-style subscriptions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/react-doctor/tests/regressions/state-rules/effect-needs-cleanup.test.ts Outdated
Comment thread packages/oxlint-plugin-react-doctor/src/plugin/constants/react.ts Outdated
@rayhanadev rayhanadev requested review from NisargIO and aidenybai and removed request for NisargIO May 25, 2026 04:29
Copy link
Copy Markdown
Member

@aidenybai aidenybai left a comment

Choose a reason for hiding this comment

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

ran the suite locally (effect-needs-cleanup 20/20, prefer-use-sync-external-store 8/8, tsc clean) and probed a few edge cases. direction is right. a handful of inline notes below, mostly small.

Comment thread packages/oxlint-plugin-react-doctor/src/plugin/constants/react.ts
@rayhanadev rayhanadev marked this pull request as draft May 25, 2026 17:51
@rayhanadev rayhanadev changed the title Recognize React Native subscription cleanup [MIL-1547] Recognize subscription.remove() as effect cleanup May 25, 2026
@rayhanadev rayhanadev requested review from aidenybai and Copilot May 25, 2026 18:13
@rayhanadev rayhanadev marked this pull request as ready for review May 25, 2026 18:20
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/utils/is-cleanup-return.ts:67

  • isCleanupReturn currently returns true for any returned subscribe-like call expression. This will incorrectly credit returns like return window.addEventListener(...) as valid cleanup (and similarly impacts expression-body effects) even though these APIs don't return a cleanup function. Suggest restricting this case to subscribe-like methods known to return an unsubscribe function, or otherwise requiring the returned value to be a function-shaped cleanup.
  if (isNodeOfType(returnedValue, "Identifier")) {
    return knownBoundReleaseNames.has(returnedValue.name);
  }
  if (isSubscribeLikeCallExpression(returnedValue)) return true;
  if (

@rayhanadev rayhanadev force-pushed the fix/rn-subscription-remove-cleanup branch from bc530c7 to c89150a Compare May 25, 2026 19:41
rayhanadev and others added 8 commits May 25, 2026 13:40
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@rayhanadev rayhanadev force-pushed the fix/rn-subscription-remove-cleanup branch from c89150a to b78bad7 Compare May 25, 2026 20:45
rayhanadev and others added 2 commits May 25, 2026 13:55
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
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.

3 participants