Repository navigation
Conversation
Normalize fetch aborts and ignore abort results during data strategy. Add integration coverage for navigation during fetcher polling.
|
Hi @yoni-noma, Welcome, and thank you for contributing to React Router! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
Add changeset for abort handling fix and sign CLA.
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
Prefer signal reason and AbortError types; only fall back to message matching for fetch TypeError cases.
Note TypeError message matching is a browser fallback only.
|
Added a short note in code: we prefer abort signal/AbortError checks and only fall back to TypeError message matching to handle browser abort race cases. The fallback is intentionally narrow (TypeError-only) to minimize false positives. Should improve behavior for Chrome/Safari/Firefox where aborted fetches sometimes surface as TypeError. |
Extract internal isAbortError helper to avoid duplication.
Ensure abort races surface AbortError consistently and add a targeted dataStrategy interruption test to guard the regression. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid treating TypeError as abort in manifest patch fetches. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep debug logging out of upstream change set. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Writing `{ type: data, data: undefined }` for aborted route results
causes mergeLoaderData to overwrite existing valid loaderData with
undefined. This crashes hooks like useRouteLoaderData('root') that
expect data to be present (e.g. "No requestInfo found in root loader").
Instead, skip aborted routes entirely so mergeLoaderData preserves
the previous valid data. The outer catch path already does this
correctly by returning empty dataResults.
Co-authored-by: Cursor <cursoragent@cursor.com>
Fix: per-route abort suppression was wiping loaderDataWe discovered a bug in the per-route abort suppression logic introduced in this PR. When an aborted route result was detected in the Impact: Hooks like Root cause: The Fix (commit e21f7ed): Skip aborted routes entirely ( This is safe because:
|
…etcher consumers
When callDataStrategy skips a route result via abort detection (isAbortError),
downstream fetcher consumers crash because they assume results always exist:
- handleFetcherLoader: result is undefined → isErrorResult(undefined) throws
- handleFetcherAction: actionResult is undefined → isRedirectResult crashes
- revalidation fetchers: result is undefined → invariant("Did not find
corresponding fetcher result") throws
Add null guards in all three consumer paths to gracefully settle fetchers
with their previous data when results are missing due to abort detection.
Co-authored-by: Cursor <cursoragent@cursor.com>
Fix: defensive null guards for skipped abort results in fetcher consumersWe identified two production errors caused by Sentry issues
Root causeWhen a browser-level abort occurs (e.g., user navigates while fetchers are in-flight), the fetch throws a
FixAdd null guards in all three consumer paths. When a result is missing (skipped by abort detection), the fetcher gracefully settles to idle with its previous data preserved — analogous to how No changes to abort detection logic — |
… in fetcher discovery error paths
The isAbortError check in fetchAndApplyManifestPatches was catching
DOMException("AbortError") even when signal.aborted was still false.
This created an inconsistency with discoverRoutes (which checks signal.aborted),
causing manifest fetches to silently return with no routes patched while
discoverRoutes proceeded as if no abort occurred — resulting in 404 errors.
Fix:
1. Revert fetchAndApplyManifestPatches to the original signal?.aborted check
2. Add isAbortError guards in handleFetcherAction and handleFetcherLoader
discovery error paths to silently suppress abort-related discovery errors
This ensures consistency between fetchAndApplyManifestPatches and discoverRoutes,
while still gracefully handling abort-related errors in the fetcher consumers.
Co-authored-by: Cursor <cursoragent@cursor.com>
Update: Manifest abort detection fix + fetcher discovery error guardsProblem discovered (WEB-APP-1G9)The
This was observed in CI (Playwright smoke tests) where rapid navigation creates tight abort timing windows. Changes in this update
Navigation callers (the two Summary of all changes in this PR
|
When the browser terminates a /__manifest fetch independently of
AbortController (e.g., rapid navigation), the fetch throws a
TypeError("Failed to fetch") while signal.aborted is still false.
Add a targeted one-time retry: yield one microtask tick to let any
pending abort() propagate, then re-check the signal. If it flipped
to aborted, treat as canceled. Otherwise retry once — real failures
still surface if the retry also fails.
This handles both navigation and fetcher callers uniformly at the
source, without requiring isAbortError guards in navigation paths.
Made-with: Cursor
…esult During navigation races, the turbo-stream response can omit data for routes that were expected, causing unwrapSingleFetchResult to throw SingleFetchNoResultError. When existing loaderData is available for the route, reuse it instead of crashing. This is consistent with how mergeLoaderData already preserves existing data for routes absent from newLoaderData. First-time loads without prior data still throw as before. Made-with: Cursor
|
👋 We've moved away from Changesets to our own internal changes process. Please convert your changesets file to a change file in the proper package directory (i.e., |
|
Any timeline when this could be released? |
i dont think anyone reviewed it, not sure why because its pretty major issue, we had multiple crashes a week, this solved 98% of it but i think it should be reviewed carefully |
|
We think we're running into this issue as well. |
Per https://github.com/remix-run/react-router/blob/main/docs/community/contributing.md#change-files, move the release note from .changeset/ to packages/react-router/.changes/ following the new naming convention (patch.<short-description>.md).
- Restore package.json version to match dev (was an inadvertent bump) - Remove stale .changeset/ files (project migrated to .changes/) - Revert prettier-style `X && X.abort()` changes that were unrelated to the fix - Add comment in fog-of-war.ts explaining the microtask yield before retry - Drop redundant `e instanceof TypeError` guard (isAbortError handles it) - Extract `unwrapSingleFetchResultWithFallback` helper to remove duplication - Add unit tests for the `isAbortError` helper
|
Refreshed the PR for review readiness:
@brophdawg11 — would really appreciate a review pass here when you have the bandwidth. This has been open since January and we're aware of at least 3 production users (us, @Tom-DK, @scolestock) hitting the underlying race. The branch is behind |
|
Any updates on this issue? We have the same issue. |
|
Same here, do you plan to merge in a near future? |
Production validation update + two related stock bugs root-causedSince the last refresh we finished a full root-cause investigation of the crash family that remained after this PR's abort fixes, and I want to close the loop here because several people in this thread are hitting it (@Tom-DK, @scolestock, @Alopwer, @davidesigner). TL;DR: the remaining crashes were not abort-shape issues — they are three separate stock fog-of-war bugs, all downstream of one design flaw: the "route not yet discovered" guards assume
Production numbers, for anyone evaluating whether this PR family is worth running as a patch: we vendor this PR's changes plus the three fixes above via pnpm patch on 7.13.0, tagged per-version in Sentry. Over the last 45 days: 6 @brophdawg11 the offer from May stands: happy to rebase this PR onto |
|
Any new news on this one? |
Description
Fixes navigation and fetcher crashes caused by inconsistent abort error shapes (
AbortError,TypeError, orundefined) in single-fetch + data strategy + manifest discovery. Normalizes abort detection in one helper (isAbortError) and applies it at the entry points where browser-level aborts can race ahead ofAbortController.abort()propagation.Related: #14203
Background
AbortSignal.abortedis not alwaystrueat the moment fetches fail. The browser can terminatefetch()at the network layer (response code 0) before JavaScript'sAbortController.abort()runs — most commonly during rapid navigation. In those windows, fetch throws aTypeError("Failed to fetch")whilesignal.abortedis stillfalse, and the error bubbles to error boundaries as a real failure.This PR introduces a shared
isAbortErrorhelper that detects abort situations across the variants browsers actually produce (AbortErrorDOMException,Errorwithname === "AbortError", orTypeErrorwith a known abort message), and routes those through the existing abort suppression paths instead of error boundaries.Changes
New shared helper
packages/react-router/lib/router/abort.ts—isAbortError(error, signal, { allowTypeError }). Strict bysignal.abortedfirst, thenAbortErrorinstances.TypeErrormessage matching is opt-in (allowTypeError: true) since it can mask real network failures if applied too broadly.Data strategy (
router.ts)mergeLoaderDatapreserves the previous valid data instead of being overwritten withundefined.callDataStrategycatch, return emptydataResultsfor abort errors instead of bubbling to root.abortPromiseincallLoaderOrActionwith a normalizedAbortError(usingsignal.reasonwhen available) so handlers can detect aborts consistently.handleFetcherAction/handleFetcherLoaderdiscovery error paths.handleFetcherAction,handleFetcherLoader, and the revalidation fetcher result merge — whencallDataStrategyskips a result for abort reasons, settle the fetcher with its previous data instead of crashing oninvariant(result, ...).abortFetcher(key, reason)so the abort source is visible insignal.reason(helps with the observability gap the original code had).Single fetch (
single-fetch.tsx)fetchAndDecodeViaTurboStreamnormalizes aTypeErrorfromfetch()into a properAbortErrorDOMException when the signal indicates an abort, so downstream consumers see the standard shape.unwrapSingleFetchResultWithFallback— whenSingleFetchNoResultErroris thrown during a navigation race, reuse existingrouter.state.loaderData[routeId]if present (mirroringmergeLoaderData's preserve-existing semantics). First-time loads without prior data still throw as before.Manifest discovery (
fog-of-war.ts)fetchAndApplyManifestPatcheswhen the initial fetch throws an abort-shapedTypeErrorbut the signal is still active. Yields a microtask to let any pendingabort()propagate, then re-checks before retrying. If the retry also fails, the error surfaces normally.signal?.aborted(noTypeErrorfallback in the outer catch), preserving consistency withdiscoverRoutes.Tests
isAbortErrorcovering signal state, AbortError instances, and theTypeErrorfallback (abort-test.ts).data-strategy-test.tsasserting aborted navigations produce anAbortErrorresult rather thanundefined.fetcher-test.tsexercising navigation during fetcher polling without surfacing errors.Known limitations
__manifestfailures in production. We're still seeing occasional manifest errors in the field — this PR reduces them substantially but does not eliminate them. Additional investigation likely needed as follow-up.SingleFetchNoResultErrorfallback; both currently rely on integration coverage from real-world usage. Happy to add focused tests if reviewers want them.Impact
Eliminates the most common abort-related crashes (
TypeError: Failed to fetch,Cannot read properties of undefined (reading 'type'),Did not find corresponding fetcher result,No result found for routeId) while leaving real navigation failures and real manifest failures visible.