Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 74600bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Size Change: +458 B (+0.41%) Total Size: 112 kB 📦 View Changed
ℹ️ View Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark React
Details
| Benchmark suite | Current: 74600bd | Previous: b091c5f | Ratio |
|---|---|---|---|
data-client: getlist-100 |
229.92 ops/s (± 6.9%) |
238.1 ops/s (± 10.3%) |
1.04 |
data-client: getlist-500 |
80 ops/s (± 4.3%) |
79.37 ops/s (± 4.9%) |
0.99 |
data-client: update-entity |
454.55 ops/s (± 7.8%) |
625 ops/s (± 9.0%) |
1.37 |
data-client: update-user |
500 ops/s (± 8.6%) |
690.48 ops/s (± 7.4%) |
1.38 |
data-client: getlist-500-sorted |
71.98 ops/s (± 10.6%) |
69.5 ops/s (± 10.0%) |
0.97 |
data-client: update-entity-sorted |
400 ops/s (± 9.7%) |
526.32 ops/s (± 6.2%) |
1.32 |
data-client: update-entity-multi-view |
526.32 ops/s (± 11.5%) |
588.24 ops/s (± 9.2%) |
1.12 |
data-client: list-detail-switch-10 |
25.97 ops/s (± 6.6%) |
20 ops/s (± 6.5%) |
0.77 |
data-client: update-user-10000 |
131.58 ops/s (± 12.8%) |
143.89 ops/s (± 11.9%) |
1.09 |
data-client: invalidate-and-resolve |
60.98 ops/s (± 5.8%) |
65.36 ops/s (± 5.9%) |
1.07 |
data-client: unshift-item |
344.83 ops/s (± 1.8%) |
400 ops/s (± 5.5%) |
1.16 |
data-client: delete-item |
488.1 ops/s (± 8.2%) |
588.24 ops/s (± 9.5%) |
1.21 |
data-client: move-item |
256.41 ops/s (± 7.2%) |
322.58 ops/s (± 8.0%) |
1.26 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 74600bd | Previous: b091c5f | Ratio |
|---|---|---|---|
normalizeLong |
430 ops/sec (±4.28%) |
423 ops/sec (±4.54%) |
0.98 |
normalizeLong Values |
385 ops/sec (±0.35%) |
389 ops/sec (±1.29%) |
1.01 |
normalizeLong Scalar |
361 ops/sec (±2.75%) |
363 ops/sec (±3.38%) |
1.01 |
normalizeLong Scalar update |
912 ops/sec (±0.15%) |
890 ops/sec (±0.85%) |
0.98 |
denormalizeLong |
226 ops/sec (±6.87%) |
234 ops/sec (±6.81%) |
1.04 |
denormalizeLong Values |
223 ops/sec (±5.47%) |
224 ops/sec (±5.19%) |
1.00 |
denormalizeLong donotcache |
994 ops/sec (±0.28%) |
980 ops/sec (±0.79%) |
0.99 |
denormalizeLong Values donotcache |
734 ops/sec (±0.54%) |
730 ops/sec (±0.57%) |
0.99 |
denormalizeLong Scalar donotcache |
1020 ops/sec (±0.74%) |
1056 ops/sec (±0.49%) |
1.04 |
denormalizeShort donotcache 500x |
1435 ops/sec (±0.07%) |
1435 ops/sec (±0.14%) |
1 |
denormalizeShort 500x |
649 ops/sec (±6.55%) |
642 ops/sec (±7.67%) |
0.99 |
denormalizeShort 500x withCache |
6921 ops/sec (±0.86%) |
6716 ops/sec (±0.34%) |
0.97 |
queryShort 500x withCache |
3210 ops/sec (±0.09%) |
3000 ops/sec (±0.14%) |
0.93 |
buildQueryKey All |
59757 ops/sec (±0.64%) |
57896 ops/sec (±1.22%) |
0.97 |
query All withCache |
6273 ops/sec (±2.14%) |
6012 ops/sec (±2.71%) |
0.96 |
denormalizeLong with mixin Entity |
208 ops/sec (±7.52%) |
212 ops/sec (±6.14%) |
1.02 |
denormalizeLong withCache |
6229 ops/sec (±0.37%) |
6567 ops/sec (±0.29%) |
1.05 |
denormalizeLong withCache (Scalar churn) |
6241 ops/sec (±0.23%) |
6519 ops/sec (±1.18%) |
1.04 |
denormalizeLong Values withCache |
5112 ops/sec (±1.27%) |
5205 ops/sec (±1.63%) |
1.02 |
denormalizeLong Scalar withCache |
7766 ops/sec (±0.25%) |
7497 ops/sec (±0.93%) |
0.97 |
denormalizeLong Scalar update withCache |
4044 ops/sec (±0.47%) |
4117 ops/sec (±0.28%) |
1.02 |
denormalizeLong All withCache |
6388 ops/sec (±0.34%) |
6088 ops/sec (±0.35%) |
0.95 |
denormalizeLong Query-sorted withCache |
6422 ops/sec (±1.46%) |
6280 ops/sec (±1.84%) |
0.98 |
denormalizeLongAndShort withEntityCacheOnly |
1667 ops/sec (±0.29%) |
1692 ops/sec (±0.37%) |
1.01 |
denormalize bidirectional 50 |
4707 ops/sec (±10.41%) |
4553 ops/sec (±10.65%) |
0.97 |
denormalize bidirectional 50 donotcache |
41885 ops/sec (±1.59%) |
43086 ops/sec (±0.20%) |
1.03 |
getResponse |
4473 ops/sec (±3.47%) |
4417 ops/sec (±4.06%) |
0.99 |
getResponse (null) |
9801823 ops/sec (±1.19%) |
9896832 ops/sec (±0.91%) |
1.01 |
getResponse (clear cache) |
203 ops/sec (±8.78%) |
205 ops/sec (±8.66%) |
1.01 |
getSmallResponse |
3547 ops/sec (±1.39%) |
3536 ops/sec (±0.25%) |
1.00 |
getSmallInferredResponse |
2781 ops/sec (±0.17%) |
2910 ops/sec (±0.31%) |
1.05 |
getResponse Collection |
4463 ops/sec (±3.49%) |
4225 ops/sec (±4.77%) |
0.95 |
get Collection |
2864 ops/sec (±0.21%) |
2596 ops/sec (±0.78%) |
0.91 |
get Query-sorted |
4980 ops/sec (±1.80%) |
5026 ops/sec (±1.69%) |
1.01 |
setLong |
438 ops/sec (±0.24%) |
414 ops/sec (±0.95%) |
0.95 |
setLongWithMerge |
250 ops/sec (±0.21%) |
246 ops/sec (±0.45%) |
0.98 |
setLongWithSimpleMerge |
264 ops/sec (±0.76%) |
260 ops/sec (±0.54%) |
0.98 |
setSmallResponse 500x |
913 ops/sec (±1.71%) |
915 ops/sec (±1.64%) |
1.00 |
setMany 50x one-per-row |
152 ops/sec (±0.36%) |
146 ops/sec (±0.89%) |
0.96 |
setMany 50 batch |
3614 ops/sec (±2.78%) |
3507 ops/sec (±2.61%) |
0.97 |
setMany 500x one-per-row |
15.7 ops/sec (±0.61%) |
15.08 ops/sec (±1.47%) |
0.96 |
setMany 500 batch |
1415 ops/sec (±3.10%) |
1419 ops/sec (±0.21%) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: FOLLOW_UP_ONLY on #4097 @ b0f6e54
Architecture holds. The two halves are not redundant: useEnhancedReducer hold keeps React-bound SET_RESPONSE / handleSet / clear tied to a fiber that actually commits (so a discarded tree cannot complete next() and wipe settledByMeta), and NetworkManager.init() re-resolve republishes into the controller that did commit when a shared manager’s held queue died with the discarded fiber. Middleware-still-immediate is the right split. throttle-only noteSettled matches the only path that enters this.fetching. Mutating settled.controller before re-resolve makes a second init() idempotent. Module-level WeakMap is fine — keys are per-FetchingMeta and never shared across manager instances. Strict Mode: heldRef goes [] → null once; remount sees null and does not double-apply. Changeset listing the linked group + use-enhanced-reducer matches .changeset/config.json. Neither half looks over-engineered relative to the failure mode.
FOLLOW_UP
- Race suite fixed waits (
precommit-race.node-suite.tsx): replacewait(100)/wait(500)with condition-based waits (text containsvalue 5/error nope, orcallsstable). Fixed sleeps will flake under load; the suite already sets a 20s timeout as a smell. renderDataHook/ActController.resolve× hold: agreed this is a helper bug, not a reason to unwind the production fix.makeRenderDataClientwrapsresolvein synchronousact(), which can race the hold’s mountuseEffect(promise microtasks vs passive effects) so the hold never opens and suspense stays pending — especially with sync fixtures. Fix the test helper (flush/open hold reliably, or stop sync-act-wrappingresolve) in a follow-up souseFetch-use.web.tsxand siblings do not timeout. Do not block this PR on that unless CI is red on HEAD.
No CHANGE_THIS_PR.
Queue actions that reach React before the mount effect and replay them from that effect. Middleware still runs immediately, and the promise from dispatch still resolves once the action commits. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
A shared NetworkManager can resolve a fetch into a controller whose store never commits. On init, publish that result through the controller that did commit, without calling the endpoint again. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
… race suite Wait for the endpoint call before releasing the blocker, then for the expected text and a stable call count. Drops the 20s suite timeout. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
b0f6e54 to
8f0f5a9
Compare
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: RE_LGTM on #4097 @ 8f0f5a9
Prior FOLLOW_UP_ONLY @ b0f6e54 is closed:
- Race suite now uses condition waits (
waitUntil/waitUntilStable) instead ofwait(100)/wait(500)—8f0f5a9a. renderDataHook/ Kick harness is on base via #4099 rebase; not required in this PR.
Production useEnhancedReducer hold + NetworkManager noteSettled/init() are unchanged vs the prior review. Two-layer architecture still holds; no CHANGE_THIS_PR. Merge when required CI (incl. Bugbot) is green on this HEAD.
React 17 and 18 have no use(), and react-dom 17 has no react-dom/client. The blocker now suspends by throwing its promise when use() is missing, and the DOM host falls back to a legacy root on React 17. Each version asserts what it actually does before the blocker releases: concurrent roots (18, 19) show no value yet, and legacy roots (17) have already committed DataProvider, so the race cannot happen there. Transition cases skip on 17, and use(useFetch()) cases skip before 19. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
Use the shared streamingHarness gate instead of a local flag plus resolver, load legacy react-dom with a typed require instead of a cast, and keep the case filter boolean. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: RE_LGTM on #4097 @ 6e0e572
Delta vs prior RE_LGTM @ 8f0f5a9 is test-only (255afaac, 6e0e572):
- Race suite now runs on React 17 and 18 with version-correct assertions: legacy roots expect the value before release (no race); concurrent roots expect it absent before release when a blocker is present (proves the park).
- Skips match reality (
startTransitionon 17,use(useFetch())before 19). - DOM host uses
ReactDOM.renderunderLegacyReact; blocker reusesmakeGatewith throw-for-17.
Production useEnhancedReducer hold + NetworkManager noteSettled/init() are byte-identical to 8f0f5a9. Two-layer architecture still holds. No CHANGE_THIS_PR; no new FOLLOW_UP. Merge when required CI (incl. Bugbot) is green on this HEAD.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4097 +/- ##
==========================================
- Coverage 98.10% 98.08% -0.03%
==========================================
Files 166 166
Lines 3167 3186 +19
Branches 626 631 +5
==========================================
+ Hits 3107 3125 +18
Misses 18 18
- Partials 42 43 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…-commit race suite Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…acy and concurrent roots Co-authored-by: Nathaniel Tucker <me@ntucker.me>
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: RE_LGTM on #4097 @ 4c130bb
Delta vs prior RE_LGTM @ 6e0e572 is test-only (b34f277, 4c130bb):
- Race suite pins fresh-manager call counts by root type (
shared || LegacyReact→ exactly one call; concurrent fresh managers still assert discard churn). - New
precommit-hold.tsxcovers the hold on legacy and concurrent roots (replay in one mount-effect render; concurrent batches promise-callback dispatches).
Production useEnhancedReducer hold + NetworkManager noteSettled/init() are byte-identical to 6e0e572. Two-layer architecture still holds. No CHANGE_THIS_PR; no new FOLLOW_UP.
Bugbot waive: the open "Outer Suspense hides provider on React 17" finding is a false positive. Same harness @ 6e0e572 had green ci/circleci: unit_tests-^17 (and ^18 / latest / 19.3 / native); ReactDOM collects precommit-race.tsx, so the LegacyReact beforeRelease assertions are already exercised and passing. Do not restructure the outer Suspense for that report.
Merge when required CI (incl. Bugbot re-run or this waive) is green on this HEAD.
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Sol review bot (GPT-6.1 Sol, High) — reviewed 4c130bb86ce252f16ae3831c1d166f96451fdcfb.
One verified P2 regression is attached: under StrictMode, a held dispatch promise can resolve before its state update commits. A head/base probe using the same React 19.2.3 DOM root reproduced resolved: ['0'], final: '2' on head and resolved: ['2'], final: '2' on base.
The existing two concurrent-root precommit-hold tests pass. I also inspected manager settlement/replay, reset handling, cleanup, and idempotent init. I did not rerun the full React/native matrix. I am not repeating the already-waived React 17 Outer Suspense report.
| const held = heldRef.current; | ||
| if (held === null) return; | ||
| heldRef.current = null; | ||
| for (const [action, resolve] of held) { |
There was a problem hiding this comment.
Sol review bot — [P2] Keep held dispatch promises pending through Strict Mode effect replay. These calls create usePromisifiedDispatch's pending promise inside the initial mount effect. In Strict Mode, React immediately replays the earlier usePromisifiedDispatch effect with the original state, which resolves that promise before the queued reducer updates commit. I reproduced this with the actual source, React/ReactDOM 19.2.3, and a StrictMode concurrent root: dispatching {n:1} and {n:2} once during render makes the second dispatch's .then() observe DOM text 0, while the eventual text is 2. Replacing only this hook with the base version makes .then() observe 2. This breaks the hook's commit-completion contract and lets callers read stale state or begin follow-up work too early. Please gate completion on a committed update (rather than a replayed mount effect), and cover this hold path under StrictMode.
Reverts "Hold dispatches until the hook commits" and its legacy and concurrent root tests. useEnhancedReducer is back to master. The fetch result is held by NetworkManager instead, in the next commit. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
Replaces "Re-resolve fetches that settled into an uncommitted store". DataProvider marks the controller it creates with awaitingInit, and initManager clears it. While it is set, NetworkManager parks a throttled fetch's resolve arguments on its in-flight record instead of dispatching into a store React may still discard. init() publishes parked results from a microtask through controller.resolve, so test controllers that wrap resolve in act() never re-enter React's commit. Parked results live on the record, so RESET and clear() drop them. Mutations resolve immediately, as before. Redux, Vue, and the server stores never set the flag. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…t race suite Expo Router's root layout is a React.lazy inside the router's own Suspense, so DataProvider's first render is a retry that the fetch's own update can restart. On master the reader stays on the fallback. Runs on every host and React version the suite already covers. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
|
Staff engineer (Cursor agent): LGTM on the new design at e02b826. One ask before merge: the PR description is out of date. Why the redesign is better. The fix now lives in one place instead of two. Things I checked:
Before merge: the description still describes the old two-part design (" CI: |
delete on a property added after construction drops V8 objects to slow properties. Every DataProvider Controller would stay that way for the app's lifetime, and its methods sit on the hottest paths. Also compare the record against the resolution's own fetchedAt. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
The tests now cover the real contract that initManager clears the flag before managers' init(), instead of a copy of it. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
cursor
Bot
changed the title
Co-authored-by: Nathaniel Tucker <me@ntucker.me>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 071b573. Configure here.
handleFetch already knows whether a fetch is in the map. publish uses that flag instead of reading sideEffect again. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
Drop the catch that swallowed a throw from the code under test, and type the dispatch mocks as SetResponseAction. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
… init() The init() microtask published parked results even if cleanup() ran first, sending them into the unmounted no-op dispatch so the in-flight record never settled. Skip publishing while cleaned up; the next init() publishes instead. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…d parked fetches Co-authored-by: Nathaniel Tucker <me@ntucker.me>
|
Reviewed the current commit gate at Two concrete follow-up test ideas:
These are coverage suggestions, not additional blocking findings. I inspected the source and CI results; local process execution was unavailable, so I did not rerun the React matrix. |
Co-authored-by: Nathaniel Tucker <me@ntucker.me>
… pre-commit race suite Co-authored-by: Nathaniel Tucker <me@ntucker.me>
… and contain a throwing resolve() init() now scans fetching and posts the Promise job only if a record holds a parked result; the job scans again so a RESET in the same flush still drops them. Each parked resolve() is wrapped so a throw becomes that fetch's error, as on the unparked path, instead of stranding the remaining parked records. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…spatch-hold-6fdf Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…nd the mock Controller Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…ommit race The race suite had its own error boundary and console.error spy. The shared ErrorBoundary and recordConsoleErrors already cover both. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSnmddZk6H7dfYTir6Sp3k
|
Docs preview: https://docs-site-pr-4097-data-client.vercel.app Built from 74600bd merged into |
Park a thunk instead of a tuple, share one settle() for the publish and its error fallback, and derive throttling from the action in publish(). Tidy the precommit race suite's case table and act-environment toggle. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSnmddZk6H7dfYTir6Sp3k
This branch was successfully deployed
3 participants
Requested by Nathaniel · project thread
Motivation
useSuspenseanduse(useFetch())can stay on the fallback when their fetch finishes beforeDataProvidercommits. React throws the uncommitted store away, so theSET_RESPONSElands nowhere. A sharedNetworkManagerthen keeps a promise that never settles, and fresh managers refetch in a loop.Two shapes cause it:
use().React.lazylayout. Expo Router's root layout works this way. The provider's first render is then a retry of a boundary that already showed its fallback, and the fetch's own update restarts that render.Solution
A commit gate in
NetworkManager. It starts open, and onlyDataProvidercloses it.Controllergets an internalawaitingInitflag.DataProvidersets it on the controller it creates, andinitManagerclears it before calling each manager'sinit(). It is cleared by assigningfalse, not withdelete, so the controller keeps V8's fast property layout.resolvearguments on its in-flightFetchingMetarecord instead of dispatching.NetworkManager.init()publishes parked results from a microtask, throughcontroller.resolve. Publishing synchronously would run inside React's passive effect flush, where test controllers that wrapresolveinact()re-enter React.RESETandclear()drop them with no extra code.useEnhancedReducerandusePromisifiedDispatchare unchanged from master.Tests
precommit-race.node-suite.tsx) runs on React DOM and React Native hosts, and on React 17, 18, and 19. It covers a blocker outside and inside the provider, a lazy layout aroundDataProviderwith and without a transition, rejected fetches, StrictMode, and a provider that settles inside a hidden<Activity>and is then revealed. On React 17 and 18, theuse(), transition, andActivitycases skip where unsupported. React 17's legacy root has no race.NetworkManager,initManager,DataProvider, andController, 22 of the 30 race tests fail on React 19.2.3 (DOM and Native). With this PR, all 30 pass.NetworkManagerunit tests drive the gate throughinitManager. They cover parking, publishing from a microtask, publishing to the controller bound atinit(), a same-turn init → cleanup → init, a middleware that throws during publish, mutations, andRESET.^17,^18,native,latest, and19.3.Bundle size
From the compressed-size bot on this PR:
rdcClient.jsrdcNextjs.jsrdcPipeableStream.jsPerformance
Same machine, interleaved, 5 rounds after a discarded warmup pair, on an Intel(R) Xeon(R) Processor.
No suite is outside that noise: every min–max range overlaps. Ratio is PR median ÷ master median (above 1 is faster). Spread is master / PR, (max − min) / median. The median of the ratios is 0.98 on the React hot path and 1.00 on Node core.
React hot path:
Node core:
The Benchmark React bot comment compares runs on different runners (Reading the CI comment).
Notes
DataProviderinside a hidden<Activity>commits without running effects, so its results stay parked until it is shown. The race suite tests this: the value or error appears on reveal with one endpoint call.🤖 Generated with Claude Code
https://claude.ai/code/session_01GSnmddZk6H7dfYTir6Sp3k
Note
Medium Risk
Changes core fetch-to-store timing for React Suspense paths; behavior is gated on
awaitingInitand covered by new race tests, but subtle timing around init/cleanup/RESET could affect edge cases.Overview
Fixes Suspense stuck on fallback (and refetch loops) when a read fetch finishes before
DataProvidercommits—e.g. a siblinguse()suspends the tree, or the provider lives inside aReact.lazylayout (Expo Router).DataProvidersetscontroller.awaitingInit = trueon the controller it creates;initManagerclears it before managers runinit(). While that flag is set,NetworkManagerdoes not dispatchSET_RESPONSEfor throttled (non-mutation) fetches: it parks the resolution on the in-flightFetchingMetaand flushes it from a microtask ininit()via newpublish/settle/publishParkedpaths. Parked work survivescleanup()until the nextinit(), is dropped onRESET, and middleware throws during publish are turned into fetch errors. Mutations still resolve immediately.Adds
NetworkManagergate tests and a sharedprecommit-racesuite (DOM + Native) covering lazy layouts, transitions, errors, StrictMode, and hidden<Activity>.Reviewed by Cursor Bugbot for commit 74600bd. Bugbot is set up for automated code reviews on this repo. Configure here.