X Tutup
Skip to content

Fix feature loading on soft navigation between React-based pages#8881

Draft
SunsetTechuila wants to merge 8 commits intorefined-github:mainfrom
SunsetTechuila:events
Draft

Fix feature loading on soft navigation between React-based pages#8881
SunsetTechuila wants to merge 8 commits intorefined-github:mainfrom
SunsetTechuila:events

Conversation

@SunsetTechuila
Copy link
Member

@SunsetTechuila SunsetTechuila commented Jan 20, 2026

@SunsetTechuila SunsetTechuila marked this pull request as ready for review January 20, 2026 15:29
@fregante
Copy link
Member

 one-event 4.4.0 published 🎉

@fregante fregante added the bug label Jan 22, 2026
}
});
} while (await oneEvent(document, 'turbo:render'));
} while (await oneEvent(document, ['turbo:render', 'soft-nav:react-done']));
Copy link
Member Author

@SunsetTechuila SunsetTechuila Jan 22, 2026

Choose a reason for hiding this comment

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

unlike soft-nav:render, soft-nav:react-done is fired when user navigates the session history too

@SunsetTechuila SunsetTechuila changed the title Load features when navigating between React-based pages Fix features loading on soft navigation between React-based pages Jan 27, 2026
@SunsetTechuila SunsetTechuila changed the title Fix features loading on soft navigation between React-based pages Fix feature loading on soft navigation between React-based pages Jan 27, 2026
document.addEventListener('turbo:before-fetch-request', unloadAll); // Clicks
document.addEventListener('turbo:visit', unloadAll); // Back/forward button
document.addEventListener('soft-nav:progress-bar:start', unloadAll); // Soft navigation
document.addEventListener('soft-nav:start', unloadAll); // Soft navigation
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit too early, but other events are fired too late

Comment on lines +131 to +132
document.addEventListener('soft-nav:start', unloadAll); // Soft navigation
addEventListener('popstate', unloadAll); // History navigation
Copy link
Member

Choose a reason for hiding this comment

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

Do you reckon that we can proceed without these two early events or do they cause too much trouble/noise in the console?

What does "firing too early" mean? Does the page break visually?

Copy link
Member Author

@SunsetTechuila SunsetTechuila Jan 28, 2026

Choose a reason for hiding this comment

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

What does "firing too early" mean? Does the page break visually?

Here is a demonstration:

msedge_prwsHOQhvG

signal.addEventListener('abort', () => {
descriptionBanner.remove();
});

signal.addEventListener('abort', () => {
disabledBanner.remove();
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I think why not unload on soft-nav:render? Seems to work well

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you reckon that we can proceed without these two early events

We can't. Neither the turbo:before-fetch-request nor the turbo:visit events are fired in the test cases I provided in the PR description

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I think why not unload on soft-nav:render?

I've let this idea marinate in my head, and I think it's too risky

@SunsetTechuila SunsetTechuila marked this pull request as draft February 3, 2026 22:47
SunsetTechuila added a commit that referenced this pull request Feb 3, 2026
To see the issue or test the fix:

1. Rebase this branch on #8881
2. Go to #4008
3. Hover the first mention
4. Click #3980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants

X Tutup