feat: centralize soft navigation handling in EmbracePageManager#1418
Conversation
joaquin-diaz
changed the title
Chrome DevTools Protocol Tracing (Script: 115.63ms, Heap: 13.20MB)
Lighthouse (Script Eval: 128.62ms)
Platform Tests (vite-7 es2015 gzip: 70.77KB)vite-6 Platform Tests
vite-7 Platform Tests
webpack-5 Platform Tests
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1418 +/- ##
=======================================
Coverage 96.73% 96.73%
=======================================
Files 234 233 -1
Lines 10126 10183 +57
Branches 1423 1436 +13
=======================================
+ Hits 9795 9851 +56
- Misses 329 330 +1
Partials 2 2
🚀 New features to boost your workflow:
|
jpmunz
left a comment
jpmunz
left a comment
There was a problem hiding this comment.
For:
Note: this removes public API surface (getNavigationInstrumentation, createReactRouterNavigationInstrumentation, NavigationInstrumentation.setCurrentRoute/setInstrumentationType) — a breaking change for any manual integrator calling these directly.
Since we're not doing a major version bump can we make these non-breaking? Since Navigation Instrumentation is being setup automatically now if we switched these to no-ops would we preserve the same behaviour as today?
For:
React-router integrations (withEmbraceRouting, withEmbraceRoutingLegacy, listenToRouterChanges) now only call page.setCurrentRoute(...) — no more dependency on NavigationInstrumentation.
Does this collide with the new soft nav detection? E.g. would we create 1 route span from the soft nav detection and then end up creating a 2nd from the React-router?
| // A route span must not outlive the session part it started in — e.g. the | ||
| // tab backgrounds, or the session ends, with no further navigation to | ||
| // trigger _onRouteChanged. Only the ending side is wired up: resuming a | ||
| // span on session-part-start was deliberately dropped in favor of relying |
There was a problem hiding this comment.
this would be a behaviour change from before then right?
If we have:
Hard load Page A -> background tab -> foreground tab -> soft navigate to Page B -> background tab
Previously we would end up with:
- SurfaceSpan1: Hard load Page A -> background tab
- SurfaceSpan2: foreground tab -> soft navigate to Page B
- SurfaceSpan3: soft navigate to Page B -> background tab
Now is the behaviour this?:
- SurfaceSpan1: Hard load Page A -> background tab
- SurfaceSpan2: soft navigate to Page B -> background tab
There was a problem hiding this comment.
That's updated now. It should behave the same as it was doing when the react instrumentations where in charge of ending the route spans
| ); | ||
| } | ||
|
|
||
| if (!config.omit?.has('navigation')) { |
There was a problem hiding this comment.
should we be allowing this to be omittable?
There was a problem hiding this comment.
Nope, not anymore given that it's doing session part rollover. We can wait for user feedback for another ways of disabling or changing how soft nav works
| Centralizing both in one place is what avoids the race that used to exist | ||
| between the session-part lifecycle and route-span creation: previously two | ||
| independent actors (the session manager reacting to the Navigation API, and | ||
| the navigation instrumentation reacting to route reports) could observe the | ||
| same soft navigation in either order. Now there's one actor driving both. |
There was a problem hiding this comment.
I think this context is not needed once we complete the refactor
| Centralizing both in one place is what avoids the race that used to exist | |
| between the session-part lifecycle and route-span creation: previously two | |
| independent actors (the session manager reacting to the Navigation API, and | |
| the navigation instrumentation reacting to route reports) could observe the | |
| same soft navigation in either order. Now there's one actor driving both. |
| - **`EmbraceUserSessionManager`** — owns session-part start/end/rollover, but | ||
| no longer knows the Navigation API exists. |
There was a problem hiding this comment.
| - **`EmbraceUserSessionManager`** — owns session-part start/end/rollover, but | |
| no longer knows the Navigation API exists. | |
| - **`EmbraceUserSessionManager`** — owns session-part start/end/rollover, no interaction with Navigation API |
| Resuming a span on the next session-part-*start* was deliberately dropped: | ||
| if the same route is still current when a new part starts, the next | ||
| `setCurrentRoute` report (or, for frameworks that only report on navigation, | ||
| none at all) is what determines whether a span reopens — not the session | ||
| part boundary itself. |
There was a problem hiding this comment.
| Resuming a span on the next session-part-*start* was deliberately dropped: | |
| if the same route is still current when a new part starts, the next | |
| `setCurrentRoute` report (or, for frameworks that only report on navigation, | |
| none at all) is what determines whether a span reopens — not the session | |
| part boundary itself. | |
| Route spans are not started on the next session-part-*start*: | |
| if the same route is still current when a new part starts, the next | |
| `setCurrentRoute` report (or, for frameworks that only report on navigation, | |
| none at all) is what determines whether a span reopens — not the session | |
| part boundary itself. |
| // span before the rollover, so the session-part-ended listener would | ||
| // incorrectly close the placeholder instead of the outgoing span. | ||
| this._userSessionManager?.rolloverSessionPartInternal({ | ||
| endReason: 'web_soft_navigation', |
There was a problem hiding this comment.
since we're rolling over wouldn't there only ever be a single reason for both the end and start?
There was a problem hiding this comment.
Yeah but I think it doesn't hurt to keep the option open for different reasons just in case
cb61ab0 to
ab81e9b
Compare
…pageManager fallback
… resumed after backgrounding
1dcc6eb to
c99c4a8
Compare
|
@jpmunz Updated the readme so it represents current state instead of how we got there or other changes |
jpmunz
left a comment
jpmunz
left a comment
There was a problem hiding this comment.
A couple follow-up comments but overall lgtm
| return; | ||
| } | ||
|
|
||
| updateZeroTimeMillis(window.performance.timeOrigin + event.timeStamp); |
There was a problem hiding this comment.
I think maybe for consistency we should have:
| updateZeroTimeMillis(window.performance.timeOrigin + event.timeStamp); | |
| updateZeroTimeMillis(epochMillisFromOrigin(event.timeStamp)); |
just so that nothing outside of the PerformanceManager is referencing timeOrigin directly
| the same reset `initSDK` performs on `pageshow` for bfcache restores. | ||
| - **React-router integrations** (`withEmbraceRouting`, | ||
| `withEmbraceRoutingLegacy`, `listenToRouterChanges`) — call | ||
| `page.setCurrentRoute({ path, url })` once they've resolved the templated |
There was a problem hiding this comment.
Does this have a possibility of colliding with the soft nav detection? E.g. if this is set from the router instrumentation first and then later the soft nav detection doesn't trigger because the url hasn't updated from the page managers perspective? If this is triggered by the router without a soft nav being detected at all would it end up creating a 2nd route span in the same session part?
There was a problem hiding this comment.
Good thinking, I had to validate this. Calling setCurrentRoute does not create a route span directly. We have two guards preventing this:
- Prevents creating a new route id in EmbracePageManager
public setCurrentRoute = (route: Route) => {
if (!this._currentRoute || this._currentRoute.url !== route.url) {
this._currentPageId = generateUUID();
}
..
- Prevents creating a route span on a same URL
private readonly _onRouteChanged = (route: Route): void => {
if (!this._config.enabled) {
return;
}
if (this._currentRouteSpan && this._currentRouteSpanUrl === route.url) {
// Same url as the currently open span: either the templated path just
// resolved (rename in place), or a redundant re-render — no-op if the
// path hasn't actually changed.
if (this._currentRouteSpanPath !== route.path) {
this._renameCurrentRouteSpan(route.path);
}
return;
}
this._endRouteSpan();
this._startRouteSpan(route);
};
2 participants
What problem is this solving?
Route (
ux.surface) span creation/ending was split across two independent, unordered actors:EmbraceUserSessionManagerreacting to the Navigation API'scurrententrychangeto roll over session parts, andNavigationInstrumentationreacting to react-router callingsetCurrentRoutedirectly. Since neither ordering was guaranteed, a soft navigation could produce a spurious extra route span or incorrectly split a route span across a session-part boundary. React-router integrations also had a direct, unnecessary dependency onNavigationInstrumentation.Note: this removes public API surface (
getNavigationInstrumentation,createReactRouterNavigationInstrumentation,NavigationInstrumentation.setCurrentRoute/setInstrumentationType) — a breaking change for any manual integrator calling these directly.Short description of changes
EmbracePageManageris now the single source of truth for the current route and the sole listener of the Navigation API. On soft navigation, it rolls over the session part (via a newly-publicrolloverSessionPartInternalonEmbraceUserSessionManager) before setting a placeholder route, so the outgoing route span and outgoing session-part span end in the correct order/batch.withEmbraceRouting,withEmbraceRoutingLegacy,listenToRouterChanges) now only callpage.setCurrentRoute(...)— no more dependency onNavigationInstrumentation.NavigationInstrumentationis simplified to a pure mirror: it reacts topage.addRouteChangedListener(same url → rename span in place, different url → end old span + start new one) and to session-part-ended (so a route span never outlives the session part it started in). It's now wired up automatically as a default instrumentation viasetupDefaultInstrumentations/initSDKinstead of being manually constructed by consumers.EmbraceUserSessionManagerno longer knows the Navigation API exists — that listening moved toEmbracePageManager.emb.instrumentationattribute on route spans (which react-router flavor reported the route); removed the now-deadEMB_NAVIGATION_INSTRUMENTATIONSenum.