-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(tanstack-router): Check for fromLocation existence before reporting pageload
#18463
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.
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
fix(tanstack-router): Check for fromLocation existence before reporting pageload
#18463
Conversation
17f7480 to
c175152
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a logic error in the TanStack Router integration that was preventing web vitals from being reported during pageload transactions. The original condition incorrectly handled the case when fromLocation is undefined during initial pageloads.
- Fixed the condition in
onBeforeNavigatehook to properly check forfromLocationexistence before comparing states - Added a test to verify pageload transactions include web vitals measurements (ttfb, lcp, fp, fcp)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react/src/tanstackrouter.ts | Fixed the logic to explicitly check for fromLocation existence before state comparison, preventing incorrect navigation span creation during pageloads |
| dev-packages/e2e-tests/test-applications/tanstack-router/tests/routing-instrumentation.test.ts | Added test to verify that pageload transactions correctly include web vitals measurements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }), | ||
| }), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test doesn't reproduce the regression being fixed (Bugbot Rules)
Per the review rules for fix PRs: the test should fail before the fix and pass after. The PR author explicitly states "I included a test anyways even though it doesn't reproduce it," meaning this test would have passed with the old buggy code. For proper regression testing, a test that specifically validates the condition where fromLocation is undefined during initial pageload and verifies no spurious navigation span is created would more directly test the fix.
nicohrubec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Bad condition which short circuits the web vitals from being reported, I included a test anyways even though it doesn't reproduce it.
closes #18425
Closes #18464 (added automatically)???