fix: prevent fetcher formData flicker and eliminate state.fetchers mutations#15028
Draft
brophdawg11 wants to merge 2 commits intodevfrom
Draft
fix: prevent fetcher formData flicker and eliminate state.fetchers mutations#15028brophdawg11 wants to merge 2 commits intodevfrom
brophdawg11 wants to merge 2 commits intodevfrom
Conversation
brophdawg11
changed the title
…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>
brophdawg11
force-pushed
the
agents/fix-bug-14506
branch
from
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. |
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
1 participant
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.