fix: prevent fetcher formData flicker and eliminate state.fetchers mutations#15028
Merged
Conversation
…ction (#14506) When a fetcher action completes and revalidation loaders run, the router was calling state.fetchers.set(key, doneFetcher) BEFORE checking for redirects and before the final updateState/completeNavigation call. This mutated the same Map object that had already been handed to React via a prior updateState() call. Because React renders startTransition updates asynchronously, if React rendered between the mutation and the next updateState(), it would see: - fetcher.formData === undefined (already idle) - loaderData === old value (not yet updated) This caused a brief flicker in optimistic UI patterns like: fetcher.formData?.get('status') ?? item.status The fix defers the fetcher idle-state assignment to the final state update (non-redirect path) so it's committed atomically alongside the new loaderData. For redirect paths the mutation is preserved (needed so the redirect navigation's completeNavigation sees the idle fetcher). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Build a new Map whenever fetcher state needs to change instead of
mutating the existing map in-place. The existing map may already have
setState) and
React renders asynchronously, so mutating it after the fact is unsafe.
Key changes:
- updateState: delete idle mountedFetchers only AFTER subscribers are
notified (so subscribers can capture idle fetcher data), not before
- HandleLoadersResult now carries an optional fetchers map so that
abortStaleFetchLoads / markFetchRedirectsDone results are committed
completeNavigation
- getUpdatedRevalidatingFetchers: build a copy, don't mutate state.fetchers
- handleFetcherAction: build updatedFetchers / finalFetchers maps instead
of mutating state.fetchers; redirect paths advance state.fetchers via
state = { ...state, fetchers: withDoneFetcher() }
- updateFetcherState: build new Map before calling updateState
- setFetcherError: build new Map before calling updateState
- deleteFetcher: no longer deletes from state.fetchers; callers that need
a React-facing snapshot must build their own Map and exclude the key
- markFetchersDone / markFetchRedirectsDone / abortStaleFetchLoads: accept
a fetchers Map parameter and operate on that instead of state.fetchers
- processLoaderData: accepts workingFetchers Map parameter
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0298a84 to
cc1bffd
Compare
Contributor
✅ Change File FoundA change file file exists in this PR. Thanks! |
Contributor
Preview Build AvailablePreview builds have been created for this PR. You can install pnpm install "remix-run/react-router#preview/pr-15028&path:packages/react-router"And/or install other packages via: pnpm install "remix-run/react-router#preview/pr-15028&path:packages/react-router-dev"
pnpm install "remix-run/react-router#preview/pr-15028&path:packages/react-router-express"
pnpm install "remix-run/react-router#preview/pr-15028&path:packages/react-router-node"
pnpm install "remix-run/react-router#preview/pr-15028&path:packages/react-router-serve"These preview builds will be updated automatically as you push new commits. |
Since state.fetchers is now always replaced with a new Map (never mutated in place), we can key useMemo on that reference. The expensive Array.from + spread only runs when fetchers actually change, not on every unrelated navigation or state update. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The original test checked subscriber states and could never thefail subscriber sees state after each updateState call, not the intermediate mutation. The real bug is that state.fetchers.set(key, doneFetcher) mutates the Map reference already handed to React's concurrent renderer. The new test captures the Map reference from the subscriber when the fetcher enters the 'loading' state, then asserts that reference was NOT mutated to 'idle' after the fetch completes. This fails on the dev branch (before the fix) with 'Expected: loading, Received: idle' and passes with the immutability refactor in place. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds four additional regression tests alongside the existing action+loader test, covering every code path that previously mutated state.fetchers in place after handing the Map reference to React: - Fetcher GET load: updateFetcherState() called twice (loading then done) mutated the loading Map via the second state.fetchers.set() call. - Fetcher revalidation during navigation: processLoaderData() mutated the loading Map (handed to subscribers during getUpdatedRevalidatingFetchers) when marking revalidating fetchers done after loaders completed. - Fetcher loader redirect: markFetchersDone() inside completeNavigation mutated the loading Map handed to subscribers before the redirect finished. - Fetcher action redirect: markFetchersDone() inside completeNavigation mutated the loading Map (post-action-redirect) handed to subscribers before the redirect navigation completed. Each test captures the Map reference from the subscriber at the relevant intermediate state, then asserts after completion that the Map still reflects that intermediate state (not the final idle state). All five tests fail on the dev branch and pass with the immutability refactor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
React's useId() can produce IDs like _r_11_ (digits only, no trailing letter), but the regex was /^_r_[0-9]?[a-z]_/ which required exactly one alpha character. Broaden to /^_r_[0-9a-z]+_/ to match any alphanumeric suffix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
brophdawg11
commented
May 12, 2026
brophdawg11
commented
May 12, 2026
Contributor
|
The preview branch |
Merged
Contributor
|
🤖 Hello there, We just published version Thanks! |
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
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.
Problem
Fixes #14506 — fetcher
formDatabriefly becomesundefinedbefore newloaderDatais available, causing UI flicker in optimistic update patterns.Root cause
In
handleFetcherAction, after the action resolved and loaders were awaited, the code called:Because
startTransitionrenders are low-priority, React could render Map M between these two lines — seeing the idleformData: undefinedfrom the done-fetcher but still with staleloaderData. This caused a one-frame flicker.Fix (commit 1)
Defer the done-fetcher assignment to the final atomic
updateStatecall:A regression test is included that uses
startTransitionsemantics to verify the fetcher is never exposed in an intermediate inconsistent state.Refactor (commit 2)
The fix revealed a broader pattern:
state.fetcherswas being mutated directly in ~11 places across 7 functions throughout the router. Any of these mutations could cause the same class of bug if React happened to render between the mutation and the nextupdateState.This commit eliminates all direct
state.fetchersmutations:updateState: delete idle mounted fetchers only after subscribers are notified (so subscribers capture idle fetcher data); the pre-notification deletion was incorrectHandleLoadersResult: added optionalfetchersfield soabortStaleFetchLoads/markFetchRedirectsDoneresults are committed atomically throughstartNavigation → completeNavigationgetUpdatedRevalidatingFetchers: builds a copy, never mutatesstate.fetchershandleFetcherAction: buildsupdatedFetchers/finalFetchersmaps; redirect paths usestate = { ...state, fetchers: withDoneFetcher() }updateFetcherState: builds new Map beforeupdateStatesetFetcherError: builds new Map beforeupdateStatedeleteFetcher: no longer deletes fromstate.fetchers; callers build their own snapshotmarkFetchersDone/markFetchRedirectsDone/abortStaleFetchLoads: accept afetchers: Mapparameter and operate on itprocessLoaderData: accepts aworkingFetchers: MapparameterTesting
All 98 fetcher unit tests pass, including the new regression test. The pre-existing
dom-export-test.tsxfailure is unrelated to these changes.