Skip to content

feat(eslint): no-unnecessary-use-callback#112689

Merged
TkDodo merged 19 commits intomasterfrom
tkdodo/feat/no-unnecessary-use-callback
Apr 15, 2026
Merged

feat(eslint): no-unnecessary-use-callback#112689
TkDodo merged 19 commits intomasterfrom
tkdodo/feat/no-unnecessary-use-callback

Conversation

@TkDodo
Copy link
Copy Markdown
Collaborator

@TkDodo TkDodo commented Apr 10, 2026

This PR adds a new lint rule, no-unnecessary-use-callback, that only checks if useCallback is unnecessary in 3 scenarios:

  1. The only usage is a direct invocation:
const fn = useCallback(() => ..., [deps])

<Component onClick={() => fn()} />

This is unnecessary because the referential stability of fn is never “consumed”.

  1. They are passed to react intrinsic elements
const fn = useCallback(() => ..., [deps])

<button onClick={fn} />

React intrinsic elements like button never expect props to be memoized, the components themselves are not memoized so the useCallback does nothing.

  1. They pare passed to scraps components. Scraps components, too, don’t expect consumers to memoize input:
const fn = useCallback(() => ..., [deps])

<LinkButton onClick={fn} />

all other cases are left as they are. Still, there were 194 violations of this new rule, which were auto fixed.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 10, 2026
# Conflicts:
#	static/app/components/replays/table/deleteReplays.tsx
# Conflicts:
#	static/app/views/explore/metrics/metricToolbar/metricSelector.tsx
#	static/app/views/performance/landing/metricsDataSwitcherAlert.tsx
#	static/app/views/settings/seer/overview/codeReviewOverviewSection.tsx
Copy link
Copy Markdown
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

This is fantastic! 👏 🔥 I'm so pumped to get this in and see the code cleanups. And hopefully eventually get it merged into React lint rule packages upstream.

No blockers, just a few small suggestions that can be taken or leaven.

if (currentGuide) {
dismissGuide(currentGuide.guide, step, orgId);
}
};
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.

[Non-Blocking] Just noting, handleDismiss is only used once, in a single callback later on that calls it and runs window.location.hash = '';. Another solution could be to move that location setter inside handleDismiss.

...but also not a blocker at all for this PR IMO. Maybe a good other lint rule to noodle on.

Comment thread static/eslint/eslintPluginSentry/no-unnecessary-use-callback.ts Outdated
if (
node.type === AST_NODE_TYPES.CallExpression &&
node.callee.type === AST_NODE_TYPES.Identifier &&
useCallbackBindings.has(node.callee.name)
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.

[Bug] Checking node names is susceptible to bugs caused by variable shadowing. It'd really be better to use

Here's an example commit with four incorrect tests: c148a13

If this were a fully shared open source project I'd request changes. But I don't think the Sentry codebase tends to do this particular shadowing weirdness. IMO it's fine for now.

Comment thread static/eslint/eslintPluginSentry/no-unnecessary-use-callback.ts Outdated
Comment thread static/eslint/eslintPluginSentry/no-unnecessary-use-callback.ts Outdated
Comment thread static/eslint/eslintPluginSentry/no-unnecessary-use-callback.ts
@sentry
Copy link
Copy Markdown
Contributor

sentry bot commented Apr 14, 2026

Sentry Snapshot Testing

Name Added Removed Modified Renamed Unchanged Status
sentry-frontend
sentry-frontend
0 0 0 0 204 ✅ Unchanged

@TkDodo TkDodo marked this pull request as ready for review April 15, 2026 09:15
@TkDodo TkDodo requested a review from a team as a code owner April 15, 2026 09:15
@TkDodo TkDodo requested review from a team as code owners April 15, 2026 09:15
@TkDodo TkDodo merged commit e3b7d0a into master Apr 15, 2026
61 checks passed
@TkDodo TkDodo deleted the tkdodo/feat/no-unnecessary-use-callback branch April 15, 2026 09:15
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 370dc0e. Configure here.


if (source.startsWith('@sentry/scraps')) {
for (const spec of node.specifiers) {
if (!scrapsExclusions.has(spec.local.name)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Exclusion check uses local name instead of imported name

Low Severity

The scrapsExclusions check uses spec.local.name (the local alias) instead of the original imported name. For aliased imports like import {CompactSelect as CS} from '@sentry/scraps/compactSelect', the exclusion check against 'CS' won't match 'CompactSelect' in the set, so CS gets added to scrapsImports. The rule would then incorrectly flag useCallback passed to <CS onChange={fn} /> as unnecessary, even though CompactSelect is explicitly excluded because it internally depends on memoized callbacks.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 370dc0e. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants