Prevent usePrefetchableForwardPaginationFragment from thrashing the server#1
Prevent usePrefetchableForwardPaginationFragment from thrashing the server#1matclayton wants to merge 4 commits into
Conversation
…erver Fixes the issues raised in facebook#5074 around automatic prefetching overwhelming the backend: 1. Stop the infinite prefetch loop on server errors. When a pagination request failed, the prefetch `useEffect` would immediately re-issue the request because none of its trigger conditions changed, hammering the server (an effective DDOS). We now track that a prefetch errored and pause automatic prefetching until the product explicitly calls `loadNext` or a `refetch` happens. This was made worse by a bug in `useLoadMoreFunction_EXPERIMENTAL` (used when `ENABLE_ACTIVITY_COMPATIBILITY` is on): on error it called `observer.complete()` instead of `observer.error()`, so the hook never learned a request had failed. Aligned it with the non-experimental variant which correctly calls `observer.error()`. 2. Add a `maxFetchSize` argument that caps the number of items requested in any single `loadMore` call (for both automatic prefetching and explicit `loadNext`). Previously the computed page size could grow to the connection's `totalCount` (several thousand items in some cases), generating queries large enough to overwhelm the backend. Both fixes are applied to `usePrefetchableForwardPaginationFragment` and its `_EXPERIMENTAL` counterpart, with new tests and updated docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UicXZPVf79LtVBMg182UX
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughTwo pagination hooks ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)packages/react-relay/relay-hooks/__tests__/usePrefetchableForwardPaginationFragment-test.jsFile contains syntax errors that prevent linting: Line 15: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 40: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 41: expected packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.jsFile contains syntax errors that prevent linting: Line 14: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 15: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 16: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 17: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 46: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 51: Expected a statement but instead found '}, ... [truncated 2228 characters] ... or remove the syntax.; Line 261: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 428: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 434: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 437: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 437: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 437: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 475: Illegal return statement outside of a function packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.jsFile contains syntax errors that prevent linting: Line 14: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 15: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 16: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 17: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 46: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 51: Expected a statement but instead found '}, ... [truncated 2228 characters] ... or remove the syntax.; Line 261: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 426: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 432: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 435: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 435: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 435: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 473: Illegal return statement outside of a function Comment |
Replace the duplicated `clampFetchSize` helper (defined identically in both the stable and _EXPERIMENTAL hooks) with an inline `Math.min(..., maxFetchSize ?? Infinity)`, removing the cross-file duplication and the extra indirection while keeping the explanatory comments at the call sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UicXZPVf79LtVBMg182UX
There was a problem hiding this comment.
⚠️ Not ready to approve
maxFetchSize currently allows non-positive values which can clamp the fetch size to 0 (sending first: 0), and the docs contain an argument-name mismatch (minimumFetchSize vs minimalFetchSize).
Pull request overview
This PR hardens usePrefetchableForwardPaginationFragment to avoid backend thrash by pausing automatic prefetching after pagination errors and by introducing an optional maxFetchSize cap to prevent overly large pagination queries.
Changes:
- Pause automatic prefetching after a pagination error until an explicit
loadNext,refetch, or reset occurs. - Add
maxFetchSizeand clamp computed pagination request sizes for both prefetching and explicitloadNext. - Fix
useLoadMoreFunction_EXPERIMENTALto propagate failures viaobserver.error()(notobserver.complete()), and add/update tests + docs.
File summaries
| File | Description |
|---|---|
| website/docs/api-reference/hooks/use-prefetchable-forward-pagination-fragment.mdx | Documents disablePrefetching, maxFetchSize, and error-pausing behavior. |
| packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.js | Implements error-pausing for prefetch + request-size clamping via maxFetchSize. |
| packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.js | Mirrors error-pausing + maxFetchSize clamping in the experimental variant. |
| packages/react-relay/relay-hooks/useLoadMoreFunction_EXPERIMENTAL.js | Fixes error propagation to observers by calling observer.error(error). |
| packages/react-relay/relay-hooks/tests/usePrefetchableForwardPaginationFragment-test.js | Adds coverage for maxFetchSize capping and for pausing/resuming prefetch after errors. |
Copilot's findings
Comments suppressed due to low confidence (2)
packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.js:61
clampFetchSizecan return 0 or a negative count whenmaxFetchSizeis set to a non-positive value (e.g.maxFetchSize={0}), which would send a pagination query withfirst: 0and likely cause confusing behavior. It should validate thatmaxFetchSizeis a positive number (and fail fast) before clamping.
// - non-nullable if the provided ref type is non-nullable
data: [readonly key: TKey] extends [
readonly key: {readonly $fragmentSpreads: unknown, ...},
]
? TData
packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.js:61
clampFetchSizecan produce a 0/negativecountif a consumer passes a non-positivemaxFetchSize(e.g.0), which then results in a pagination query withfirst: 0. Add an invariant to ensuremaxFetchSizeis a positive number before clamping.
// - non-nullable if the provided ref type is non-nullable
data: [readonly key: TKey] extends [
readonly key: {readonly $fragmentSpreads: unknown, ...},
]
? TData
- Files reviewed: 5/5 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.js (1)
263-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the prefetch-error pause for every accepted
loadNext.Line 281 only runs when the explicit call needs a network fetch. If a failed prefetch leaves some data buffered, a subsequent explicit
loadNextcan consume that buffer without clearinghasPrefetchErroredRef, so automatic prefetching stays paused until a later network-needed call.Proposed fix
// Matches the behavior of `usePaginationFragment`. If there is a `loadMore` ongoing, // the hook handles making the `loadMore` a no-op. if (!isLoadingMoreRef.current || availableSizeRef.current >= 0) { + // An explicit `loadNext` is a deliberate user action, even when it can + // be fulfilled from the buffer. + hasPrefetchErroredRef.current = false; + // Preemtively update `availableSizeRef`, so if two `loadMore` is called in the same tick, // a second `loadMore` can be no-op availableSizeRef.current -= numToAdd; @@ // If the product needs more items from network, load the amount needed to fullfil // the requirement and cache, capped at the current amount defined by product if (!isLoadingMoreRef.current && availableSizeRef.current < 0) { - // An explicit `loadNext` is a deliberate user action, so clear any - // previous prefetch error and attempt to fetch again. - hasPrefetchErroredRef.current = false; loadMore(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.js` around lines 263 - 282, The hasPrefetchErroredRef.current flag is only cleared when a network fetch is needed (inside the nested if statement checking if availableSizeRef.current is less than 0), but it should be cleared for every accepted loadMore call in the showMore function. Move the line hasPrefetchErroredRef.current = false outside of the nested if (!isLoadingMoreRef.current && availableSizeRef.current < 0) block but keep it within the outer if (!isLoadingMoreRef.current || availableSizeRef.current >= 0) block, so that prefetch errors are cleared on every explicit loadNext call, preventing automatic prefetching from remaining paused when buffered data from a failed prefetch is consumed.packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.js (1)
263-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the prefetch-error pause for every accepted
loadNext.Line 281 only runs when the explicit call needs a network fetch. If a top-up prefetch failed while there is still buffered data, the next explicit
loadNextcan be fulfilled from the buffer and leavehasPrefetchErroredRefset, so automatic prefetching remains paused despite the deliberate product action.Proposed fix
// Matches the behavior of `usePaginationFragment`. If there is a `loadMore` ongoing, // the hook handles making the `loadMore` a no-op. if (!isLoadingMoreRef.current || availableSizeRef.current >= 0) { + // An explicit `loadNext` is a deliberate user action, even when it can + // be fulfilled from the buffer. + hasPrefetchErroredRef.current = false; + // Preemtively update `availableSizeRef`, so if two `loadMore` is called in the same tick, // a second `loadMore` can be no-op availableSizeRef.current -= numToAdd; @@ // If the product needs more items from network, load the amount needed to fullfil // the requirement and cache, capped at the current amount defined by product if (!isLoadingMoreRef.current && availableSizeRef.current < 0) { - // An explicit `loadNext` is a deliberate user action, so clear any - // previous prefetch error and attempt to fetch again. - hasPrefetchErroredRef.current = false; loadMore(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.js` around lines 263 - 282, The hasPrefetchErroredRef flag is only cleared inside the nested if block checking if (!isLoadingMoreRef.current && availableSizeRef.current < 0), which means it only gets cleared when an explicit loadNext call requires a network fetch. However, if a previous prefetch failed but buffered data is still available, the next explicit loadNext can be fulfilled entirely from the buffer without triggering a network fetch, leaving hasPrefetchErroredRef set to true and keeping automatic prefetching paused. Move the line hasPrefetchErroredRef.current = false; outside of the nested if condition so that it executes for every explicit loadNext call in the showMore callback, regardless of whether the request can be fulfilled from the buffer or requires a network fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-relay/relay-hooks/__tests__/usePrefetchableForwardPaginationFragment-test.js`:
- Around line 1285-1348: The current test 'should resume prefetching after an
error once `loadNext` is explicitly called' only covers the case where loadNext
must fetch from the network after an error. Add a new test variant that
exercises the cache-served path: set up a scenario where a top-up prefetch fails
while leaving buffered edges available, then call loadNext(1) which should be
fulfilled from that buffer, and verify that automatic prefetching still resumes
afterward. This ensures the resume functionality works both for network-fetched
and cache-served loadNext calls.
In
`@packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.js`:
- Around line 57-62: The clampFetchSize function does not validate the
maxFetchSize parameter before using it in the Math.min calculation, which allows
invalid values like 0, negative numbers, or NaN to be returned. Add validation
to ensure maxFetchSize is a valid positive number before clamping, and only
apply the minimum constraint if maxFetchSize passes validation; otherwise,
return the original fetchSize to prevent zero-size or invalid requests in
automatic prefetching.
In
`@packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.js`:
- Around line 57-62: The clampFetchSize function does not validate that
maxFetchSize is a positive number before using it in Math.min, which can result
in returning 0, negative values, or NaN. Add validation inside the
clampFetchSize function to check that maxFetchSize is both not null and is a
valid positive number (greater than 0). If maxFetchSize fails validation, return
fetchSize unchanged instead of clamping it. This prevents edge cases where a
misconfigured maxFetchSize causes automatic prefetching to issue first: 0
queries that receive no edges and trigger redundant prefetches.
In
`@website/docs/api-reference/hooks/use-prefetchable-forward-pagination-fragment.mdx`:
- Line 91: The documentation for the `maxFetchSize` parameter does not specify
that it must be a positive integer, which could lead callers to pass 0 thinking
it disables fetching. Update the `maxFetchSize` parameter documentation to
explicitly state that the value must be a positive integer (greater than 0),
clarifying the contract and preventing invalid pagination requests from
zero-sized fetch attempts.
---
Outside diff comments:
In
`@packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.js`:
- Around line 263-282: The hasPrefetchErroredRef.current flag is only cleared
when a network fetch is needed (inside the nested if statement checking if
availableSizeRef.current is less than 0), but it should be cleared for every
accepted loadMore call in the showMore function. Move the line
hasPrefetchErroredRef.current = false outside of the nested if
(!isLoadingMoreRef.current && availableSizeRef.current < 0) block but keep it
within the outer if (!isLoadingMoreRef.current || availableSizeRef.current >= 0)
block, so that prefetch errors are cleared on every explicit loadNext call,
preventing automatic prefetching from remaining paused when buffered data from a
failed prefetch is consumed.
In
`@packages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.js`:
- Around line 263-282: The hasPrefetchErroredRef flag is only cleared inside the
nested if block checking if (!isLoadingMoreRef.current &&
availableSizeRef.current < 0), which means it only gets cleared when an explicit
loadNext call requires a network fetch. However, if a previous prefetch failed
but buffered data is still available, the next explicit loadNext can be
fulfilled entirely from the buffer without triggering a network fetch, leaving
hasPrefetchErroredRef set to true and keeping automatic prefetching paused. Move
the line hasPrefetchErroredRef.current = false; outside of the nested if
condition so that it executes for every explicit loadNext call in the showMore
callback, regardless of whether the request can be fulfilled from the buffer or
requires a network fetch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38e5d26a-1bf8-419b-a6aa-e0a31ca18702
📒 Files selected for processing (5)
packages/react-relay/relay-hooks/__tests__/usePrefetchableForwardPaginationFragment-test.jspackages/react-relay/relay-hooks/useLoadMoreFunction_EXPERIMENTAL.jspackages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment.jspackages/react-relay/relay-hooks/usePrefetchableForwardPaginationFragment_EXPERIMENTAL.jswebsite/docs/api-reference/hooks/use-prefetchable-forward-pagination-fragment.mdx
Addresses review feedback on the maxFetchSize / error-pause changes: - Guard against a non-positive (or NaN) `maxFetchSize`. Previously a `maxFetchSize` of 0 would clamp every request to `first: 0`, which makes no progress and re-introduces the infinite prefetch loop the cap exists to prevent. Such values are now normalized to "no cap" via a single `effectiveMaxFetchSize` derived once per render. - Clear the prefetch-error pause on every accepted `loadNext`, not just the ones that issue a network request. A `loadNext` served entirely from the buffer is still a deliberate user action and should re-enable automatic prefetching. - Docs: correct the `minimalFetchSize` argument name (was `minimumFetchSize`) and document that `maxFetchSize` must be a positive integer. - Tests: cover the buffer-served post-error resume path and the non-positive `maxFetchSize` behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UicXZPVf79LtVBMg182UX
Summary
Addresses the production issues raised in facebook/relay#5074 where
usePrefetchableForwardPaginationFragment's automatic prefetching can overwhelm the backend.1. Stop the infinite prefetch loop on server errors (the "DDOS")
When a pagination request errored, the prefetch
useEffectre-issued the request immediately because none of its trigger conditions (hasNext, buffer not full, etc.) changed on failure. The result was a tight retry loop hammering a server that's already struggling — effectively a self-inflicted DDOS.We now track when a prefetch request has errored (
hasPrefetchErroredRef) and pause automatic prefetching until the product takes a deliberate action:loadNextcall, orrefetch, orThis was compounded by a real bug in
useLoadMoreFunction_EXPERIMENTAL(the code path used whenENABLE_ACTIVITY_COMPATIBILITYis enabled): on error it calledobserver.complete()instead ofobserver.error(), so the hook never found out a request had failed. It's now aligned with the non-experimentaluseLoadMoreFunction_CURRENT, which correctly callsobserver.error().2. Add a
maxFetchSizeargument (page-size cap)Previously the computed page size could grow up to the connection's
totalCount(several thousand items in some production cases), generating a single query large enough to overwhelm the backend. There was aminimalFetchSizebut no upper bound.A new optional
maxFetchSizeargument clamps the number of items requested in any singleloadMore— both automatic prefetching and explicitloadNext. When set, the hook fills the buffer incrementally (in chunks of at mostmaxFetchSize) instead of in one giant request. When unset, behavior is unchanged.Changes
usePrefetchableForwardPaginationFragment.jsandusePrefetchableForwardPaginationFragment_EXPERIMENTAL.js: error-pausing of prefetch + newmaxFetchSizeparam +clampFetchSizehelper.useLoadMoreFunction_EXPERIMENTAL.js: report errors viaobserver.error()instead ofobserver.complete().maxFetchSizecapping (prefetch +loadNext), prefetch pausing after an error, and resuming on explicitloadNext.maxFetchSize,disablePrefetching, and the error behavior.Testing
usePrefetchableForwardPaginationFragment-test.js— 14/14 pass (4 new).usePaginationFragmentanduseBlockingPaginationFragmentsuites pass (no regressions from the shareduseLoadMoreFunction_EXPERIMENTALchange).flow check— no errors. Prettier clean.🤖 Generated with Claude Code
https://claude.ai/code/session_019UicXZPVf79LtVBMg182UX
Generated by Claude Code
Summary by CodeRabbit
maxFetchSizeto cap pagination request sizes for both manual loading and automatic prefetching.disablePrefetchingto fetch only via explicitloadNext.loadNextorrefetch.