⚗️ Add soft navigation LCP tracking for route_change views - #4966
Open
mormubis wants to merge 20 commits into
Open
⚗️ Add soft navigation LCP tracking for route_change views#4966mormubis wants to merge 20 commits into
mormubis wants to merge 20 commits into
Conversation
…kspace app The nuxt-vue-router-v4-app is a generated app (gitignored, not registered in the root Yarn workspace). Playwright's webServer config starts every listed server before any test run, regardless of -g filtering. When Playwright spawned `yarn dev` in that directory, Yarn 4 traversed up to the workspace root and failed with "Package for nuxt-vue-router-v4-app@workspace:. not found" because the package isn't registered there. Switch to `yarn start` (nuxt preview of the pre-built .output/) which runs the built app directly via node and does not trigger Yarn workspace resolution.
…ntries, tighten E2E assertion, fix spec drift
mormubis
commented
Aug 20, 2026
| export interface RumInteractionContentfulPaintTiming { | ||
| entryType: RumPerformanceEntryType.INTERACTION_CONTENTFUL_PAINT | ||
| interactionId: number | ||
| largestContentfulPaint: RumLargestContentfulPaintTiming |
Contributor
Author
There was a problem hiding this comment.
Reused RumLargestContentfulPaintTiming here instead of a flat shape. The WICG spec types InteractionContentfulPaint.largestContentfulPaint as the full LargestContentfulPaint interface, not a subset, so this felt more accurate. Also gets toJSON() for free this way.
mormubis
commented
Aug 20, 2026
Bundles Sizes Evolution
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: fbb0da9 | Docs | View more details | Give us feedback! |
…soft-nav LCP test - Ran 'yarn format' on 3 files that were failing the format check (never ran yarn format locally during development, only yarn lint/typecheck). - The 'reports LCP' E2E test only skipped on non-chromium browsers, but chromium-pinned (Chrome 120) normalizes to browserName 'chromium' too and predates the Soft Navigation API (needs Chrome 151+). Switched to test.info().project.name for an exact match, following the same pattern already used in salesforce.scenario.ts.
- Only buffer pending ICP entries until the soft-navigation entry is known; once resolved, applyIcpEntries handles updates live and the buffer was never read again, so it was just accumulating for no reason. - Fix chromium-pinned being silently skipped in the 'does not error on browsers without the soft navigation API' test, same browserName vs project.name normalization issue already fixed elsewhere. It's a real Chromium build without the API, a better fit for this test than relying on firefox/webkit alone.
|
Super excited to see this! Let me know if you have any questions or anything I can do to help. |
Reuses the existing loadDelay/loadTime/renderDelay breakdown from trackLargestContentfulPaint by extracting computeLcpSubParts and parametrizing its baseline instead of hardcoding TTFB from the navigation entry. For a soft nav there's no request/response round trip to measure TTFB from, so the soft-navigation entry's own startTime is used as the baseline instead, per web-vitals' own approach (assume TTFB = 0). No behavior change for the initial-load path, verified by its existing test suite staying green unchanged. Addresses the open review comment/discussion with @tunetheweb.
…reference Kept the one genuinely non-obvious invariant (setViewEnd must unsubscribe immediately or an ended view could steal the next view's entry). Dropped the reference to the spec doc's 'Per-view vs global subscription' section, that doc was removed from the PR earlier so it was a dead pointer.
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90e9abce2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…es a soft nav setViewEnd() only unsubscribed the soft-navigation observer, leaving the ICP one alive for the full 5-minute stop() window. For a view whose interaction never produces a soft-navigation entry, softNavEntry never gets set, so the existing !softNavEntry guard on the ICP buffer push never turns off, and every ICP entry firing anywhere on the page keeps getting buffered for nothing. On interaction-heavy SPAs with frequent route changes this adds up across every unmatched view. Once setViewEnd() runs with no softNavEntry, we know for certain it never will (the soft-nav subscription is dead too), so unsubscribe the ICP one immediately as well. Added activeObserverCount to mockPerformanceObserver to make the teardown itself testable -- the JS-visible behavior is identical with or without this fix, since correlation already short-circuits on a missing softNavEntry, so the only way to prove the fix is checking the observer was actually disconnected. Found by Codex automated review.
…te soft-nav entry If this view's own transition never produces a soft-navigation entry (rare) and a later, unrelated interaction on the same view does (e.g. a query-only history update, which areDifferentLocation() in trackViews.ts intentionally doesn't treat as a new RUM view), that entry gets attributed to the wrong tracker. Considered a real fix (correlate by URL, needs new plumbing since newView() doesn't receive location today) and a timeout-based mitigation (bounds the window but doesn't close the race). Both add real complexity for a compounding edge case Codex itself flagged as P2. Accepting it for now, feature is experimental/flag-gated, revisit if it shows up in practice. Found by Codex automated review.
…ect names Fixes the same class of bug for the third time in this file (chromium vs chromium-pinned via browserName, then via project.name, now android). The android Playwright project uses current Chromium under a different project name, so the project.name === 'chromium' check missed it, and the test would have failed there once android is added to CI (it isn't yet). Asking the browser directly whether it supports the API -- the same check the SDK itself uses internally (supportPerformanceTimingEvent) -- avoids needing to know about every current-Chromium project name up front. Found by Codex automated review.
mormubis
marked this pull request as ready for review
September 7, 2026 10:49
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbb0da98e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const lcpEntry = entry.largestContentfulPaint | ||
| const resourceUrl = lcpEntry.url || undefined | ||
| const largestContentfulPaint: LargestContentfulPaint = { | ||
| value: (lcpEntry.startTime - softNavEntry.startTime) as RelativeTime, |
There was a problem hiding this comment.
issue: When a soft navigation is interrupted by the tab being backgrounded or the device sleeping before its content paints, an ICP entry can arrive much later and this subtraction publishes the entire pause as LCP. The initial-view path explicitly rejects entries at or beyond LCP_MAXIMUM_DELAY to avoid sleep-mode and browser outliers (trackLargestContentfulPaint.ts), but this route path has no equivalent relative-delay check, so opted-in SPAs can send arbitrarily large route-change LCP values and skew aggregates; reject derived values beyond the same limit.
Useful? React with 👍 / 👎.
allspain
approved these changes
Sep 7, 2026
|
|
||
| /** | ||
| * Tracks LCP for a `route_change` view via Chrome's Soft Navigation API (gated behind | ||
| * ExperimentalFeature.SOFT_NAVIGATION + browser support, see trackViews.ts). |
Collaborator
There was a problem hiding this comment.
Is the idea that this will be default behavior in v8? If so we should leave it in the comments
Comment on lines
+77
to
+80
| expect(routeChangeViews[0].view.performance?.lcp).toBeUndefined() | ||
|
|
||
| // No console errors -- automatically validated by test teardown. | ||
| }) |
Contributor
There was a problem hiding this comment.
Suggested change
| expect(routeChangeViews[0].view.performance?.lcp).toBeUndefined() | |
| // No console errors -- automatically validated by test teardown. | |
| }) | |
| expect(routeChangeViews[0].view.performance?.lcp).toBeUndefined() | |
| }) |
I think this log is not necesary.
Comment on lines
271
to
285
|
|
||
| const { stop: stopInitialViewMetricsTracking, initialViewMetrics } = | ||
| loadingType === ViewLoadingType.INITIAL_LOAD | ||
| ? trackInitialViewMetrics(configuration, startClocks, setLoadEvent, scheduleViewUpdate) | ||
| : { stop: noop, initialViewMetrics: {} as InitialViewMetrics } | ||
| const { | ||
| stop: stopInitialViewMetricsTracking, | ||
| initialViewMetrics, | ||
| setViewEnd: setRouteChangeViewEnd, | ||
| } = loadingType === ViewLoadingType.INITIAL_LOAD | ||
| ? { ...trackInitialViewMetrics(configuration, startClocks, setLoadEvent, scheduleViewUpdate), setViewEnd: noop } | ||
| : loadingType === ViewLoadingType.ROUTE_CHANGE && | ||
| isExperimentalFeatureEnabled(ExperimentalFeature.SOFT_NAVIGATION) && | ||
| supportPerformanceTimingEvent(RumPerformanceEntryType.SOFT_NAVIGATION) | ||
| ? trackRouteChangeViewMetrics(configuration, scheduleViewUpdate) | ||
| : { stop: noop, initialViewMetrics: {} as InitialViewMetrics, setViewEnd: noop } | ||
|
|
||
| // Start BFCache-specific metrics when restoring from BFCache | ||
| if (loadingType === ViewLoadingType.BF_CACHE) { |
Contributor
There was a problem hiding this comment.
Suggested change
| const { stop: stopInitialViewMetricsTracking, initialViewMetrics } = | |
| loadingType === ViewLoadingType.INITIAL_LOAD | |
| ? trackInitialViewMetrics(configuration, startClocks, setLoadEvent, scheduleViewUpdate) | |
| : { stop: noop, initialViewMetrics: {} as InitialViewMetrics } | |
| const { | |
| stop: stopInitialViewMetricsTracking, | |
| initialViewMetrics, | |
| setViewEnd: setRouteChangeViewEnd, | |
| } = loadingType === ViewLoadingType.INITIAL_LOAD | |
| ? { ...trackInitialViewMetrics(configuration, startClocks, setLoadEvent, scheduleViewUpdate), setViewEnd: noop } | |
| : loadingType === ViewLoadingType.ROUTE_CHANGE && | |
| isExperimentalFeatureEnabled(ExperimentalFeature.SOFT_NAVIGATION) && | |
| supportPerformanceTimingEvent(RumPerformanceEntryType.SOFT_NAVIGATION) | |
| ? trackRouteChangeViewMetrics(configuration, scheduleViewUpdate) | |
| : { stop: noop, initialViewMetrics: {} as InitialViewMetrics, setViewEnd: noop } | |
| // Start BFCache-specific metrics when restoring from BFCache | |
| if (loadingType === ViewLoadingType.BF_CACHE) { | |
| let viewMetricsTracking = | |
| loadingType === ViewLoadingType.INITIAL_LOAD | |
| ? { ...trackInitialViewMetrics(configuration, startClocks, setLoadEvent, scheduleViewUpdate), setViewEnd: noop } | |
| : { stop: noop, initialViewMetrics: {} as InitialViewMetrics, setViewEnd: noop } | |
| if ( | |
| loadingType === ViewLoadingType.ROUTE_CHANGE && | |
| isExperimentalFeatureEnabled(ExperimentalFeature.SOFT_NAVIGATION) && | |
| supportPerformanceTimingEvent(RumPerformanceEntryType.SOFT_NAVIGATION) | |
| ) { | |
| viewMetricsTracking = trackRouteChangeViewMetrics(configuration, scheduleViewUpdate) | |
| } | |
| const { | |
| stop: stopInitialViewMetricsTracking, | |
| initialViewMetrics, | |
| setViewEnd: setRouteChangeViewEnd, | |
| } = viewMetricsTracking | |
| // Start BFCache-specific metrics when restoring from BFCache | |
| if (loadingType === ViewLoadingType.BF_CACHE) { |
What do you think to improve readability?
Comment on lines
+38
to
+54
| createTest('does not report LCP on a route_change view without the experimental feature enabled') | ||
| .withRum() | ||
| .withBody(NAV_BUTTON) | ||
| .run(async ({ intakeRegistry, flushEvents, page, browserName }) => { | ||
| test.skip(browserName !== 'chromium', 'Soft navigation API is Chromium-only') | ||
|
|
||
| await page.locator('#nav-button').click() | ||
| await page.waitForTimeout(100) | ||
|
|
||
| await flushEvents() | ||
|
|
||
| const routeChangeViews = intakeRegistry.rumViewEvents.filter((v) => v.view.loading_type === 'route_change') | ||
| expect(routeChangeViews.length).toBeGreaterThanOrEqual(1) | ||
|
|
||
| const lastRouteChange = routeChangeViews[routeChangeViews.length - 1] | ||
| expect(lastRouteChange.view.performance?.lcp).toBeUndefined() | ||
| }) |
Contributor
There was a problem hiding this comment.
I believe this is tested in the unit tests, no need to add a e2e IMO.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
route_changeviews never report LCP today.trackInitialViewMetricsonly runs forinitial_load, so SPAs get zero LCP after the first page. Chrome's Soft Navigation API (stable since Chrome 151, no flag needed on their end) actually solves this now:soft-navigation+interaction-contentful-paintentries let us compute LCP per client-side navigation.There's an open GitHub issue for this (#2696) and an earlier draft PR (#4154) that only added a boolean
is_soft_navigationflag without touching LCP, which isn't really what the issue is asking for. This PR does the actual LCP computation.Changes
New
ExperimentalFeature.SOFT_NAVIGATIONflag. When enabled and the browser supports it,route_changeviews get atrackRouteChangeViewMetricstracker (same shape astrackInitialViewMetrics) that subscribes tosoft-navigationandinteraction-contentful-paintentries, correlates them byinteractionId, and fills inview.performance.lcp.*. No schema changes, reuses the existing field.The tricky part was figuring out when to stop listening for the
soft-navigationentry. It's per-view and Chrome fires it async, so a view whose interaction never actually produced a soft navigation would keep listening and could steal the next view's entry if I didn't unsubscribe it in time. Left a comment on that.Test instructions
enableExperimentalFeatures: ['soft_navigation']in your init config.history.pushStatein the same task (that's the heuristic Chrome uses to detect a soft navigation).route_changeview event,view.performance.lcp.timestampshould be populated.undefinedlike before.Unit tests:
yarn test:unit --spec packages/browser-rum-core/src/domain/view/viewMetrics/trackRouteChangeViewMetrics.spec.tsandyarn test:unit --spec packages/browser-rum-core/src/domain/view/trackViews.spec.tsE2E:
yarn test:e2e -g "soft navigation"(Chromium only). Confirmed green in CI across all browser variants.Checklist