⚗️ Add soft navigation LCP tracking for route_change views - #4966
⚗️ Add soft navigation LCP tracking for route_change views#4966mormubis wants to merge 18 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
| export interface RumInteractionContentfulPaintTiming { | ||
| entryType: RumPerformanceEntryType.INTERACTION_CONTENTFUL_PAINT | ||
| interactionId: number | ||
| largestContentfulPaint: RumLargestContentfulPaintTiming |
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.
| } | ||
| biggestIcpSize = entry.largestContentfulPaint.size | ||
| const lcpEntry = entry.largestContentfulPaint | ||
| const largestContentfulPaint: LargestContentfulPaint = { |
There was a problem hiding this comment.
No subParts here on purpose. That breakdown (loadDelay/loadTime/renderDelay) needs a TTFB baseline from the hard nav, which doesn't really apply to a soft navigation. Left it undefined for now. Would it make more sense to have a soft-nav-specific breakdown eventually, or is undefined fine long term?
There was a problem hiding this comment.
While I admit that without a TTFB baseline (which is.... tricky to define for soft navs), loadDelay gets merged with the actual TTFB, I think there's still value in knowing if it takes a long time to be discovered (which as I say includes TTFB), load or render. So we included subparts in web-vitals library for soft navs, just with assuming TTFB is 0.
The other argument is that, for an SPA, it's easier to reduce any LCP resource loadDelay (with prefetching ether in advance, on hover, or as part of click ) so TTFB is less relevant. While it's true the LCP resource may not be known until any route fetch completes, that's still a "loadDelay" that could be worked around if the improvements made this worthwhile.
Bundles Sizes Evolution
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 483ecd2 | 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.
|
@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".
| if (!softNavEntry) { | ||
| // Buffer only until we know which interaction to correlate against -- once softNavEntry is | ||
| // set, applyIcpEntries below handles everything live and this buffer is never read again. | ||
| pendingIcpEntries.push(...entries) |
There was a problem hiding this comment.
issue: When the feature is enabled and a route_change is not detected as a soft navigation—for example, a programmatic startView() or pushState()—setViewEnd() disconnects only the soft-navigation observer, while this callback remains active for the five-minute delayed-stop period. Because softNavEntry can no longer be set, every subsequent ICP entry is appended here but never consumed, and every recently ended unmatched view retains another copy. On interaction-heavy SPAs with frequent route changes this causes avoidable, multiplying memory retention; stop the ICP subscription or disable this buffering when such a view ends.
Useful? React with 👍 / 👎.
| if (softNavEntry) { | ||
| return | ||
| } | ||
| softNavEntry = entries[0] |
There was a problem hiding this comment.
issue: When a route_change tracker has not received its own soft-navigation entry, it unconditionally claims the next entry observed. This can misattribute metrics after a history transition that creates a RUM view but produces no soft-navigation entry, followed by a user-initiated query-only History API update: areDifferentLocation() in trackViews.ts intentionally ignores location.search, so no new RUM view is created for the latter update, but Chrome can still emit a soft-navigation entry and this older tracker attaches its LCP to the wrong view. Correlate the entry with the view's URL/timing rather than assuming the first entry belongs to the tracker.
Useful? React with 👍 / 👎.
| test.skip( | ||
| test.info().project.name === 'chromium', | ||
| 'This test validates behavior on browsers without soft-navigation API' | ||
| ) |
There was a problem hiding this comment.
issue: The Playwright configuration registers an android project using the current Chromium-based Pixel 7 device, but this skip excludes only the project literally named chromium. Consequently, the test runs on Android's current Chromium even though that engine supports the soft-navigation API, and the enabled tracker can populate LCP, contradicting the assertion at line 77. Skip all current Chromium projects or detect API support in the page so the unsupported-browser scenario does not fail in the Android matrix.
Useful? React with 👍 / 👎.
…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.
2 participants
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