Skip to content

[react-dom] Enforce small gap between completed navigation and default Transition indicator #33354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 28, 2025
Merged

[react-dom] Enforce small gap between completed navigation and default Transition indicator #33354

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32d1e4b
Add affordance to trigger Chrome crash
eps1lon May 25, 2025
f87e7a4
[react-dom] Enforce 100ms delay before default Transition indicator i…
eps1lon May 25, 2025
f8ab64e
Always delay by 20ms
eps1lon May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions fixtures/flight/src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import Button from './Button.js';
import Form from './Form.js';
import {Dynamic} from './Dynamic.js';
import {Client} from './Client.js';
import {Navigate} from './Navigate.js';

import {Note} from './cjs/Note.js';

Expand Down Expand Up @@ -89,6 +90,7 @@ export default async function App({prerender}) {
<Note />
<Foo>{dedupedChild}</Foo>
<Bar>{Promise.resolve([dedupedChild])}</Bar>
<Navigate />
</Container>
</body>
</html>
Expand Down
40 changes: 40 additions & 0 deletions fixtures/flight/src/Navigate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
'use client';

import * as React from 'react';
import Container from './Container.js';

export function Navigate() {
/** Repro for https://issues.chromium.org/u/1/issues/419746417 */
function provokeChromeCrash() {
React.startTransition(async () => {
console.log('Default transition triggered');

await new Promise(resolve => {
setTimeout(
() => {
history.pushState(
{},
'',
`?chrome-crash-419746417=${performance.now()}`
);
},
// This needs to happen before React's default transition indicator
// is displayed but after it's scheduled.
100 + -50
);

setTimeout(() => {
console.log('Default transition completed');
resolve();
}, 1000);
});
});
}

return (
<Container>
<h2>Navigation fixture</h2>
<button onClick={provokeChromeCrash}>Provoke Chrome Crash (fixed)</button>
</Container>
);
}
23 changes: 19 additions & 4 deletions packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

const defaultTransitionIndicatorDelay = 100;

export function defaultOnDefaultTransitionIndicator(): void | (() => void) {
if (typeof navigation !== 'object') {
// If the Navigation API is not available, then this is a noop.
Expand All @@ -16,6 +18,10 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) {
let isCancelled = false;
let pendingResolve: null | (() => void) = null;

const scheduledAt = performance.now();
// Delay the start a bit in case this is a fast Transition.
setTimeout(startFakeNavigation, defaultTransitionIndicatorDelay);

function handleNavigate(event: NavigateEvent) {
if (event.canIntercept && event.info === 'react-transition') {
event.intercept({
Expand All @@ -29,6 +35,7 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) {
}

function handleNavigateComplete() {
const wasRunning = pendingResolve !== null;
if (pendingResolve !== null) {
// If this was not our navigation completing, we were probably cancelled.
// We'll start a new one below.
Expand All @@ -38,7 +45,18 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) {
if (!isCancelled) {
// Some other navigation completed but we should still be running.
// Start another fake one to keep the loading indicator going.
startFakeNavigation();
if (wasRunning) {
// Restart sync to make it not janky if it was already running
startFakeNavigation();
Copy link
Collaborator

@sebmarkbage sebmarkbage May 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this cause the Chrome bug to happen anyway in this scenario?

Copy link
Collaborator Author

@eps1lon eps1lon May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to only crash if we start the spinner from navigationcomplete triggered by replaceState. If the spinner is already running, starting the fake sync navigation is fine. Probably because the earlier pendingResolve call will only stop the spinner in the next task?

Copy link
Collaborator

@sebmarkbage sebmarkbage May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird. I wonder if there's a race condition though that

Regardless, doesn't this trigger the spinner to restart? So maybe it's worth waiting a little to see if we get unblocked to avoid restarting it? Since it'll glitch regardless if we restart but not if we can avoid it.

Copy link
Collaborator Author

@eps1lon eps1lon May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, doesn't this trigger the spinner to restart?

It doesn't actually for the case where another navigation completes before this Transition finished: navigationcomplete triggers while navigation.transition is still set even though it finishes right after in navigation.transition.finished.then. So we might have to add listeners to the navigation.transition.finished Promise as well.

Anyway, the current implementation can't be used as it crashes Chrome. So while I would definitely wait for Chrome to acknowledge the issue before landing this in Canary, I'd still merge this so that we can at least continue experimenting with it.

In the meantime I come up with some more synthetic scenarios that increase our coverage

} else {
// Since it hasn't started yet, we want to delay it up to a bit.
// There needs to be an async gap to work around https://issues.chromium.org/u/1/issues/419746417.
const fakeNavigationDelay = Math.max(
Copy link
Collaborator

@sebmarkbage sebmarkbage May 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually don't trust Math.max because it can be patched it is not automatically optimized like syntax can be. This is not a hot path but I try to avoid it as a pattern so that people don't copy paste it elsewhere.

Copy link
Collaborator Author

@eps1lon eps1lon May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nobody ever patches globals 😄

Copy link
Collaborator

@sebmarkbage sebmarkbage May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that even if it's not patched, it can't be optimized as well because the VM can't assume it won't be. At least not without compromises.

0,
defaultTransitionIndicatorDelay - (performance.now() - scheduledAt),
Copy link
Collaborator

@sebmarkbage sebmarkbage May 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the rationale here is a bit inversed.

If an unrelated navigation happens in beginning happen then there already is a spinner showing. The rationale for delaying it doesn't make us avoid a spinner from being shown at all since it was already triggered. The reason for the original default delay doesn't apply in this scenario because there was already one going. In fact, delaying it just makes it feel like there was two separate actions instead of one sequence.

On the other hand if this happens after the original one has already started, and a different navigation comes in to briefly interrupt and then we want to resume, it might be good to avoid restarting if we're about to finish.

So I feel like in either scenario there should be a small gap to let any pending tasks that unblocks finish it before we start but not so long to let it feel like a separate action.

Basically, this should just always be like 20ms whether we interrupted the first start or was already running and restarted.

Copy link
Collaborator Author

@eps1lon eps1lon May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, delaying it just makes it feel like there was two separate actions instead of one sequence.

Is that just because we entangle Transitions currently and won't let them commit independently? Because I feel like there is a desire to let two interleaved Transitions commit independently and in that model we'd want two spinners instead of one sequence?

If an unrelated navigation happens in beginning happen then there already is a spinner showing.

We don't know if there's already a spinner because history.(push|replace)State trigger navigationcomplete.

Basically, this should just always be like 20ms whether we interrupted the first start or was already running and restarted.

Wouldn't that make it more likely that the spinner is flashing?

Copy link
Collaborator

@sebmarkbage sebmarkbage May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I feel like there is a desire to let two interleaved Transitions commit independently and in that model we'd want two spinners instead of one sequence?

Maybe if we restarted it upon the start of each Transition so it stops and restarts every time you start a new action but even then I'd think we'd still want to entangle the stop in case a later Transition ends before an earlier. Doesn't really address this scenario.

The thing to avoid is the scenario when the spinner starts disconnected from any interaction like a click. That's when it feels like something happened outside the user's control and something just started automatically and it's unclear what I'm waiting on.

When there are no other Navigations on the page that aren't coordinated by a React Transition, (e.g. pushState in useLayoutEffect should be ok), we can avoid that scenario.

In that case this should only happen in the scenario where you have unrelated navigations on the page or things like MPA navs (including bfcache navs that might restore the page back into a loading state).

Another scenario it can happen is if you are implementing a router using the Navigation API for push navigation. Now we don't actually recommend that because the Navigation API doesn't support the commit: 'after-transition' option yet, but if you insist we should support it.

In that scenario the spinner would potentially first restart upon the Navigation start. If an Action is still ongoing after the Navigation finishes, we should restart the indicator so that it keeps going.

A common scenario might for example be that upon click you start a React Transition and then in the same click but in a later macrotask, you start the Navigation such as if you listen to document.body clicks. In that scenario, the React Navigation would start and immediate stop due to the actual Navigation that comes later. Then let's say that Navigation takes 50ms to complete, it has already started a spinner so there's no reason for us to wait another 50ms before we restart it.

Now if the site instead did the same thing with the legacy pushState API then maybe there wasn't a spinner to begin with since it's immediately updating and in that case maybe it would be ok to wait but I feel like it's worse to wait longer in the Navigation API scenario to make it feel like it's auto-navigating without user input.

We could maybe detect it by tracking when navigations start and stop to see how long a spinner might have been running. If it was immediate, then we know we can wait but given that the code size of this default implementation is sensitive since everyone pays for it whether they use it or not. Doesn't seem worth it.

So that's the reason to favor 0ms or 20ms over 100ms.

Whether 0ms or 20ms is better for the immediate restart is a bit difficult. There's two reasons to favor 20ms. One is just that if you have programmatic logging etc observing the navigations, then you have less interference from fake ones so that's a reason to try to avoid them if we can.

If the UI causes a restart of a spinner or if the loading progress line goes back to zero upon restart then that's a reason to favor the 20ms to avoid that scenario when possible. However, if the UI doesn't cause a restart when we use 0ms but instead treats that as just a continuation of the on-going navigation in the browser UI then it's better to do that since that's our ideal anyway.

The problem is we don't know all the UIs that will use this since Safari and Firefox hasn't implemented it and various Chromium browsers might do their own thing.

I believe Chrome restarts the spinner so that's the reason I'd favor 20ms but 0ms is probably ok too given that if we're going to actually be able to finish the transition it's likely in the next macrotask which is already scheduled and so the timer happens after anyway.

);
setTimeout(startFakeNavigation, fakeNavigationDelay);
}
}
}

Expand Down Expand Up @@ -70,9 +88,6 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) {
}
}

// Delay the start a bit in case this is a fast navigation.
setTimeout(startFakeNavigation, 100);

return function () {
isCancelled = true;
// $FlowFixMe
Expand Down
Loading