Conversation
Soft navigation CLS, LCP, and INP are joined to their navigation span through the interaction that triggered the navigation. The join only worked in one direction: `spanStart` parked the span in `_pendingNavigation`, and the Event Timing handler consumed it. An entry that arrived before the span saw no pending navigation, skipped, and was never reconsidered, so its `interactionId` never reached `_interactionIdToNavigationSpan` and all three vitals for that navigation were dropped. The two events race and neither is under the SDK's control. Entry delivery follows the paint after the interaction, while the navigation span starts from framework router code on the main thread. Under load the router code can slip behind the paint. Make the join work from either side. Entries with no matching pending navigation now go into a capped list, and `spanStart` claims a matching one before parking the span. The match rule and the 5ms tolerance are unchanged, so this does not loosen what counts as a match. It only drops the requirement that the span be registered first. This also fixes a second miss. It would discard any entry that failed the match against the current `_pendingNavigation`. A navigation whose entry never arrived left a stale pending span behind, and the next navigation's early entry was then thrown away against it. ref: #24354, #24366 fix: #24480 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Left the two prior issues as |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d78bb0f. Configure here.
size-limit report 📦
|
| _lastInteractionTimestamp != null ? { span, interactionTimestamp: _lastInteractionTimestamp } : undefined; | ||
| _pendingNavigation = undefined; | ||
| const interactionTimestamp = _lastInteractionTimestamp; | ||
| if (interactionTimestamp == null || performance.now() - interactionTimestamp > MAX_INTERACTION_AGE_MS) { |
There was a problem hiding this comment.
This check also runs on the span-first path, and it's measured from event.timeStamp, so a click with a handler that takes >1.5s never binds.
I tried a 2s handler in Chrome 153, the span still starts before the entries and Chrome emits the soft-nav with the same interactionId, but we'd drop all three vitals. on develop branch this doesn't happen and those are the INPs we need to catch because they would represent really bad values.
Can we drop the check?
| * the interaction a navigation happens during can match it, so a handful is plenty, and the cap | ||
| * stops a page with many interactions and no navigations from growing the list. | ||
| */ | ||
| const MAX_UNBOUND_INTERACTIONS = 20; |
There was a problem hiding this comment.
spanStart only looks for _lastInteractionTimestamp, which only moves forward, so an entry that doesn't match it on arrival can never be claimed.
We could keep a single slot instead of the list and drop the cap and its test.
| performanceHandlers.clear(); | ||
| vi.stubGlobal('PerformanceObserver', { supportedEntryTypes: ['event', 'soft-navigation'] }); | ||
| // Pinned so the fixtures' interaction timestamps below stay inside `MAX_INTERACTION_AGE_MS`. | ||
| vi.spyOn(performance, 'now').mockReturnValue(1500); |
There was a problem hiding this comment.
Pinning this to 1500 for every test hides the span-first case with an old click.
Can we add a click at 1000, performance.now() at 3500, startSpan, then the entry, and expect it to bind?
|
I didn't want to overcomplicate the correlation when I was working on this because web vitals will move to metrics soon which would allow for looser correlation. Still if we already see those in tests then we should fix them, I think the approach is sound in general even if it doesn't fix the flakes but I think we have a few cases I pointed out where we drop some vitals that we should instead catch. |
Soft navigation CLS, LCP and INP are joined to their navigation span through the interaction that triggered the navigation. The join only worked in one direction:
spanStartparked the span in_pendingNavigation, and the Event Timing handler consumed it. An entry that arrived before the span saw no pending navigation, skipped, and was never reconsidered, so itsinteractionIdnever reached_interactionIdToNavigationSpanand all three vitals for that navigation were dropped.The two events race and neither is under the SDK's control. Entry delivery follows the paint after the interaction, while the navigation span starts from framework router code on the main thread. Under load the router code can slip behind the paint. Vue is the most exposed, because
vueIntegrationstarts the span from arouter.beforeEachguard.Make the join work from either side. Entries with no matching pending navigation now go into a capped list, and
spanStartclaims a matching one before parking the span. The match rule and the 5ms tolerance are unchanged, so this does not loosen what counts as a match. It only drops the requirement that the span be registered first.This also fixes a second miss the old code had. It discarded any entry that failed the match against the current
_pendingNavigation. A navigation whose entry never arrived left a stale pending span behind, and the next navigation's early entry was then thrown away against it.Two guards keep the new list from misattributing a vital, since an unconsumed entry now outlives the moment it arrived. A navigation span cannot claim an interaction that another navigation span already claimed, which matters because one interaction delivers several entries and only one of them binds. And a navigation span cannot claim an interaction older than 1.5s, so a programmatic
router.pushcannot inherit the last click on the page however long ago it happened. That window is the onebrowserTracingIntegrationalready uses to decide whether a navigation followed a click.ref: #24354, #24366
Fixes #24480