fix(client-react): stop five hooks from looping on dependency identity (#4693, #4694) - #4701
Merged
Merged
Conversation
…hook behavior (#4682) `packages/client-react` shipped 8 public hooks with `build` and `typecheck` as its only scripts and zero test files. `tsc --noEmit` is structurally blind to what actually breaks in a hook: a dependency array is a value, not a type, so a missing entry, a missing cleanup and a callback that never fires all typecheck perfectly. #4678 was exactly that shape — `useAutoRefresh` ignored predicate writes, the case that dirties a list hardest, and no type noticed. Adds the workspace's first DOM test environment (jsdom + @testing-library/react, `environment: 'jsdom'`; every other package runs `node`) and 17 tests over the realtime hooks covering the three things the type checker cannot see: - re-subscription driven by the dependency array — changing `object` opens a subscription on the new name and releases the old one, an unrelated re-render churns nothing, and the hooks key on the primitive `options?.recordId` / `options?.packageId` rather than the options object's identity, so an equal-but-new object stays a no-op; - release on unmount, including `useAutoRefresh`, which holds two subscriptions; - delivery — events reach state and callbacks, and `useAutoRefresh` refetches on the per-record *and* the bulk stream (the #4678 regression pin). Every assertion was verified by sabotage rather than assumed: dropping the `object` dep (1 failure), deleting a cleanup (3) and reverting `useAutoRefresh` to the single-stream version (3) each turn the suite red; reverting turns it green again. CI needs no wiring — `Test Core` partitions by package off `turbo ls`, and the new `test` script puts client-react on shard 2 (verified with scripts/partition-test-shards.mjs). No runtime code changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYnZrTwXbrctB8E8HpJAPT
…ding them `check:type-check-coverage` failed on the first push, and it was right. The initial commit added `**/*.test.tsx` to tsconfig's `exclude` to match the `**/*.test.ts` entry every sibling package carries — but that entry is frozen DEBT the repo is migrating away from (#4311), not a convention to copy. The gate's own summary makes it explicit: 21 packages still exclude their tests, carrying 2243 frozen raw errors in TEST_DEBT. Excluding one more would have reported green over source `tsc` never read. Drops the test exclusion entirely, so client-react typechecks its own tests and stays out of that ledger. Doing so immediately surfaced a real gap: the `METADATA_EVENT` fixture was declared and never used, because `useMetadataSubscription` was covered for re-subscription and unmount but not for delivery. Adds that assertion rather than deleting the fixture — 18 tests now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYnZrTwXbrctB8E8HpJAPT
#4693, #4694) Five hooks keyed a `useCallback`/`useEffect` on values the caller supplies inline — `where`/`fields`/`orderBy` objects, `onSuccess`/`onError` handlers, and the `fetcher` `useMetadata` takes as a required positional argument. Inline means a fresh identity every render, so the effect re-ran every render; because the fetch hooks call `setState`, that render caused another. Under the hooks' own documented usage this was an unbounded request loop. Requests issued in 250ms by one mounted component, before → after: useQuery (inline where) 4691 → 1 useInfiniteQuery (inline where) 6611 → 1 useObject (NO options at all) 4306 → 1 useView (inline onSuccess) 8197 → 1 useMetadata (inline fetcher) 7654 → 1 useObject and useMetadata needed no particular usage to loop: the former depended on its own `data`/`etag` state while writing both, the latter takes its fetcher positionally so there is no non-inline way to call it. useMutation was never affected — no effect drives it. The same root cause churned the realtime subscriptions (#4694): useAutoRefresh with an unmemoized `refetch` — which is exactly what useQuery returned every render — resubscribed on both streams every render, losing any event delivered in the unsubscribe/resubscribe gap. Adds two internal primitives (not exported): `stableKey` derives a dependency from a structural VALUE (sorted keys, array order preserved, since `orderBy` is positional) so a rebuilt-but-equal object is a no-op; `useEventCallback` gives a handler a fixed identity while always invoking its latest version, synced in an effect rather than during render so a discarded concurrent render cannot publish a handler that never committed. Fixing this by asking callers to memoize was rejected: the TSDoc examples themselves pass object literals, and correctness must not rest on every call site remembering `useMemo`. 13 tests, each verified by reverting the fix it guards — restoring identity deps in useQuery (3 red), useObject's self-referential state (3), useMetadata's fetcher dep (2) and the subscription callback dep (2). Counts are asserted exactly rather than as an upper bound, which would pass on a loop that merely got slower. Coverage includes the inverse direction: a changed value still refetches, and every stabilized handler runs its newest version, so the ref indirection cannot silently trade a loop for a stale closure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYnZrTwXbrctB8E8HpJAPT
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
Base automatically changed from
claude/issue-4682-client-react-test-harness
to
main
August 2, 2026 19:21
github-actions
Bot
added
documentation
os-zhuang
marked this pull request as ready for review
August 2, 2026 19:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4693
Fixes #4694
问题
五个 hook 把
useCallback/useEffect的依赖挂在调用方内联传入的值上——where/fields/orderBy对象、onSuccess/onError处理器,以及useMetadata作为必传位置参数接收的fetcher。内联意味着每次渲染都是新身份,于是 effect 每渲染重跑;而取数 hook 会setState,那次渲染又触发下一次。在这些 hook 自己文档示例的写法下,这就是一个无界请求循环。
单个已挂载组件在 250ms 内发出的请求数:
useQuery(内联where)useInfiniteQuery(内联where)useObject(完全不传 options)useView(内联onSuccess)useMetadata(内联fetcher)useObject和useMetadata不需要任何特定用法就会循环:前者的依赖数组里有它自己的data和etagstate,而函数体又写这两个;后者的 fetcher 是位置参数,根本没有非内联的调用方式。useMutation不受影响——没有 effect 驱动它。这是实测确认的(0 次调用),不是假设。同一根因也在 churn 实时订阅(#4694):
useAutoRefresh拿到未 memo 的refetch(而 #4693 修复前useQuery每渲染返回的正是这种),会每渲染在两条流上各退订重订一次,退订与重订之间到达的事件直接丢失。改动
两个内部原语(不从 index 导出),分别解决两半:
stableKey(value)—— 把结构化值变成「值变了才变」的依赖。键排序({a,b}与{b,a}等价),数组顺序保留(orderBy: ['-created_at','name']的顺序是语义)。跟随仓库既有先例(service-settings/service-automation各有一份本地实现)。useEventCallback(fn)—— 给处理器一个恒定身份,同时始终调用最新版本。ref 在 effect 里同步而非渲染期写入:并发渲染下被丢弃的那次渲染不应该把一个从未提交的处理器发布出去。没有选择「让调用方自己
useMemo」:TSDoc 示例本身传的就是对象字面量,把正确性押在每个调用点记得 memo 上等于没修。测试:13 条,每条都用还原修复的方式验过
useQuery依赖改回按身份useObject自身 state 放回依赖useMetadata的fetcher放回依赖callback请求数断言用的是精确值而不是
< 10之类的上界——后者在「循环只是变慢了」的情况下依然会通过,而那正是这套测试要防的失败。覆盖也包含反方向,免得修复把循环换成更糟的东西:
where从{status:'open'}变成{status:'done'}必须发起第二次请求;同值重建的对象则必须是 no-op。onSuccess不触发重取,但下一次取数必须打到新的那个;订阅回调换掉后,送达的事件必须进新回调而不是订阅时捕获的旧闭包。验证
按真实退出码(
cmd | tail的退出码是tail的,不作为证据),并在并入 main 之后重跑过一遍:顺带一提
仓库没有配
react-hookseslint 插件,而exhaustive-deps正是专门抓这类缺陷的规则。这解释了它们为何能长期存活。是否引入该插件是个独立的取舍(它会在既有代码上产生大量告警),没有夹带进本 PR。🤖 Generated with Claude Code
https://claude.ai/code/session_01LYnZrTwXbrctB8E8HpJAPT