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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 📦
|
|
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. |
fix the three concerns raised by @logaretm
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (_pendingInteraction?.interactionTimestamp === interactionTimestamp) { | ||
| _interactionIdToNavigationSpan.set(_pendingInteraction.interactionId, span); | ||
| _pendingInteraction = undefined; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: A stale _pendingInteraction can be incorrectly claimed by a later programmatic navigation because the promised 1.5-second staleness check is missing, leading to incorrect span association.
Severity: MEDIUM
Suggested Fix
Implement a staleness check before associating a navigation span with a _pendingInteraction. When a navigation span starts, compare the current timestamp with _pendingInteraction.interactionTimestamp. Only associate the span if the interaction occurred within a reasonable window (e.g., 1.5 seconds) to prevent claiming arbitrarily old interactions.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/browser-utils/src/web-vitals/softNavs.ts#L130-L134
Potential issue: A stale `_pendingInteraction` from a user click can persist
indefinitely. If a programmatic navigation (e.g., via `router.push`) occurs much later
without any intermediate user interaction, it will incorrectly claim this old
interaction. This happens because the check at line 130 compares
`_pendingInteraction.interactionTimestamp` with a timestamp derived from
`_lastInteractionTimestamp`, which is never reset or checked for staleness. As a result,
a navigation span can be associated with an unrelated, arbitrarily old user click,
leading to incorrect performance metrics. This contradicts the intended behavior
described in the pull request, which specified a 1.5-second window for claiming
interactions.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
I think it may flake in the future because all of this is not exact matching, it all depends on the timing and the windows are pretty tight.
But until we switch to metrics, this won't be 100% solvable or until browsers ship a sync correlation ID that we can use. But still, I think this is a good fix to have, thanks!
This branch has not been deployed
2 participants
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. An entry that finds no pending navigation is now held in
_pendingInteraction, andspanStartclaims it 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.A single slot is enough.
spanStartmatches against_lastInteractionTimestamp, which only moves forward, so an entry that does not match it when it arrives can never match it later. Entries that do match all belong to one interaction and carry oneinteractionId.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.One guard keeps the held interaction from being mis-attributed, since it now outlives the moment it arrived. A navigation span cannot claim an interaction that another navigation span already claimed. This matters because one interaction delivers several entries and only the first of them binds, so without the guard a later navigation with no interaction of its own could take the leftovers.
ref: #24354, #24366
Fixes #24480