Skip to content

Commit 52be861

Browse files
authored
fix(browser-utils): Ensure web vital client hooks unsubscribe correctly (#17272)
So this was a fun one to track down 😅 In our standalone span web vitals code, we register multiple client hooks to listen e.g. for `afterStartPageloadSpan` events. This hook will only fire once (by design), so we want to unsbscribe from it afterwards. Howerver, we register two callbacks (one for LCP, once for CLS). Because we used to unsubscribe synchronously from within the client hook callback, we synchronously removed the callback from the client's hooks array. This synchronous array mutation (shrinking) caused the second callback to no longer be executed. This surfaced by the LCP span being sent but the CLS span not being sent, due to the CLS span's hook callback no longer being called. This PR fixes this incorrect unsubscription by deferring the unsubscription calls to the next tick. This way, the array mutation no longer happens synchronously and all remaining callback hooks are invoked correctly. If you're confused by this, rest assured, I was too 😅 Happy to explain better/in-person on request :D closes https://linear.app/getsentry/issue/JS-811/investigate-missing-standalone-cls-spans-in-latest-sdk-versions
1 parent 5bf18c4 commit 52be861

File tree

1 file changed

+11
-3
lines changed
  • packages/browser-utils/src/metrics

1 file changed

+11
-3
lines changed

packages/browser-utils/src/metrics/utils.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -226,13 +226,21 @@ export function listenForWebVitalReportEvents(
226226
// we only want to collect LCP if we actually navigate. Redirects should be ignored.
227227
if (!options?.isRedirect) {
228228
_runCollectorCallbackOnce('navigation');
229-
unsubscribeStartNavigation?.();
230-
unsubscribeAfterStartPageLoadSpan?.();
229+
safeUnsubscribe(unsubscribeStartNavigation, unsubscribeAfterStartPageLoadSpan);
231230
}
232231
});
233232

234233
const unsubscribeAfterStartPageLoadSpan = client.on('afterStartPageLoadSpan', span => {
235234
pageloadSpanId = span.spanContext().spanId;
236-
unsubscribeAfterStartPageLoadSpan?.();
235+
safeUnsubscribe(unsubscribeAfterStartPageLoadSpan);
237236
});
238237
}
238+
239+
/**
240+
* Invoke a list of unsubscribers in a safe way, by deferring the invocation to the next tick.
241+
* This is necessary because unsubscribing in sync can lead to other callbacks no longer being invoked
242+
* due to in-place array mutation of the subscribers array on the client.
243+
*/
244+
function safeUnsubscribe(...unsubscribers: (() => void | undefined)[]): void {
245+
unsubscribers.forEach(u => u && setTimeout(u, 0));
246+
}

0 commit comments

Comments
 (0)