Skip to content

Review 5269

Jump to bottom
Cindy Zhang edited this page Aug 24, 2026 · 1 revision

#5269 — lint: forbid inline mergeRefs calls

nynexman4464 (internal) · OPEN · view on GitHub

Verdict: request changes — merging turns main red.

Head reviewed: d9b991f5ac8de381f126d79eb15658b97ad8ceb9 · CONFLICTING/DIRTY.

Problem

mergeRefs(a, b) called inline in JSX builds a new callback identity every render, so any child memoized on its ref prop re-renders every time. #5266 and #5267 migrated core to useMergedRefs; nothing stops the next one being written.

Solution

An error-level eslint rule, @astryx/no-inline-merge-refs, in the internal plugin, plus README and index registration.

Impact

Contributors to packages/core — a new inline mergeRefs now fails lint. No consumer-visible change; internal/ is private: true and absent from every published build.

API

No public API change. Adds one rule id to the internal plugin.

Theme targets

No new theme targets.

Breaking

  • API — no. Visual — no. Theme — no.
  • CI — yes, and this is the finding: main has a violation.

Performance & resources

One more rule in the lint pass; measured cost is noise against the existing suite.

Risk class

Needs judgement — an error-level gate whose current catch is non-zero on main.

Judgement

request changes

1. Main has one inline `mergeRefs` left, added after this branch was cut
   → merging turns main red on the next lint run. `TouchDateField` landed in
     #5350 with `ref={mergeRefs(ref, inputRef)}`; it is the ONLY ESLint error in
     packages/core/src when the rule runs against main today. CI is green on this
     PR because it last ran on a three-day-old merge base
                                          · TouchDateField.tsx:1172 (on main)

2. [not blocking] the README asserts the tree is clean of the pattern
   → prose stating the state of the tree with nothing re-checking it; already
     false against main            · internal/eslint-plugin-astryx/README.md:126

3. [not blocking] a hoisted `const mergeRefs(...)` churns identity the same way
   and the test marks it valid
   → someone lands an unmemoized merged ref and the rule says nothing.
     Avatar.tsx:651 and Button.tsx:762 both do this
                                        · no-inline-merge-refs.test.mjs:37

R14b: the rule fires exactly once on main today (verified by git grep), which is the finding. R7b: schema: [], no allowlist to drift; the five hand-maintained mirrors of plugin.rules are all updated.

Correction to this loop's own earlier note on the PR: it said the risk was open branches going red and cited 52 files. That count predated #5267 by 3.5 hours and the exposure is main itself, not branches.

The review, as posted

Thanks — the rule is right, and the two PRs under it landed clean.

Correcting my own earlier note: I said the risk was open branches going red. It's main. TouchDateField landed in #5350 after you branched and carries the exact pattern — ref={mergeRefs(ref, inputRef)} at TouchDateField.tsx:1172. Run the rule against main today and that is the only ESLint error in packages/core/src. CI is green here because it last ran on a three-day-old merge base.

One for later, not this PR: Avatar.tsx:651 and Button.tsx:762 build the same unstable callback with a hoisted const, which your test marks valid.

Merge main and move that one line to useMergedRefs — then it's error-clean and I'd land it.

What changed before posting

Inlines dropped — the anchors sit outside the diff hunks.

Clone this wiki locally