Skip to content

fix(prefetch): Fix "tap" prefetch strategy when view transitions are enabled #14235

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

Jump to bottom
Merged
merged 12 commits into from
Oct 9, 2025
Merged

fix(prefetch): Fix "tap" prefetch strategy when view transitions are enabled #14235

merged 12 commits into from
Oct 9, 2025

Conversation

@toxeeec
Copy link
Contributor

@toxeeec toxeeec commented Aug 16, 2025 •
edited
Loading

Changes

The "tap" prefetch strategy was the only strategy that didn't call onPageLoad to attach event listeners to new elements. This caused "tap" to only work on the first clicked link when view transitions were enabled.

Testing

New tests are added to e2e/prefetch.test.js that check if each prefetch strategy works correctly with view transitions

Docs

No docs added since this is a bugfix.

@changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: cfa9165

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 16, 2025
@toxeeec toxeeec changed the title Tap view transitions fix(prefetch): Fix "tap" prefetch strategy when view transitions are enabled Aug 16, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 16, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #14235 will not alter performance

Comparing toxeeec:tap-view-transitions (cfa9165) with main (28b2a1d)1

Summary

✅ 6 untouched

Footnotes

  1. No successful run was found on main (3bb14b7) during the generation of this report, so 28b2a1d was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@ematipico
Copy link
Member

ematipico commented Aug 21, 2025 •
edited
Loading

This behavior isn’t explicitly tested. No new tests were added since this PR only unifies "tap" with the other strategies.

It is tested in our e2e testing suite: https://github.com/withastro/astro/blob/main/packages/astro/e2e/prefetch.test.js

Can you please add a new test that covers the fix, please?

@toxeeec
Copy link
Contributor Author

toxeeec commented Aug 30, 2025

This behavior isn’t explicitly tested. No new tests were added since this PR only unifies "tap" with the other strategies.

It is tested in our e2e testing suite: https://github.com/withastro/astro/blob/main/packages/astro/e2e/prefetch.test.js

Can you please add a new test that covers the fix, please?

I meant that the exact behavior was not tested (prefetching multiple times with view transitions enabled), but now it is

@ematipico
Copy link
Member

It seems that the test isn't passing?

@toxeeec
Copy link
Contributor Author

toxeeec commented Sep 12, 2025

It seems that the test isn't passing?

@ematipico should be fixed now

martrapp
martrapp reviewed Sep 30, 2025
View reviewed changes
@florian-lefebvre florian-lefebvre added the needs response Issue needs response from OP label Oct 2, 2025
@toxeeec toxeeec requested a review from martrapp October 9, 2025 04:20
martrapp
martrapp approved these changes Oct 9, 2025
View reviewed changes
Copy link
Member

@martrapp martrapp left a comment

Choose a reason for hiding this comment

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

LGTM!

and thank you very much for the thorough test coverage!

@martrapp martrapp merged commit c4d84bb into withastro:main Oct 9, 2025
22 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 9, 2025
Merged
florian-lefebvre added a commit that referenced this pull request Oct 13, 2025
@florian-lefebvre @github-actions
@matthewp @renovate @astrobot-houston @toxeeec @martrapp @ArmandPhilippot @sarah11918 @abdo-spices @penx
Update next (#14530)
f135f5b 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Florian Lefebvre <[email protected]>
Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: Houston (Bot) <[email protected]>
Co-authored-by: Bartosz Kapciak <[email protected]>
Co-authored-by: Bartosz Kapciak <[email protected]>
Co-authored-by: Armand Philippot <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Abdelrahman Abdelfattah <[email protected]>
Co-authored-by: Alasdair McLeay <[email protected]>
Fix failing x-forwarded-host tests (#14505)
fix(prefetch): Fix "tap" prefetch strategy when view transitions are enabled (#14235)
fix `security.allowedDomains` version (#14509)
Fix compatibility with older Astro versions in @astrojs/node (#14514)
Fixes #14513
fix heading level in config reference docs (#14517)
fix(deps): update all non-major dependencies (#14522)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martrapp martrapp martrapp approved these changes

Assignees

No one assigned

Labels

needs response Issue needs response from OP pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@toxeeec @ematipico @martrapp @florian-lefebvre