ref(browser): Restructure browser-utils by domain - #23071
Open
logaretm wants to merge 1 commit into
Open
Conversation
logaretm
force-pushed
the
awad/webvitals-restructure
branch
from
August 5, 2026 16:47
af2fd8a to
cc6aaf8
Compare
Contributor
size-limit report 📦
|
logaretm
force-pushed
the
awad/webvitals-restructure
branch
from
August 5, 2026 17:35
cc6aaf8 to
071799f
Compare
logaretm
force-pushed
the
awad/webvitals-restructure
branch
2 times, most recently
from
August 5, 2026 19:00
e1990b4 to
794433e
Compare
logaretm
force-pushed
the
awad/webvitals-restructure
branch
from
August 5, 2026 19:16
794433e to
7a246f5
Compare
Reorganize browser-utils/src away from the catch-all metrics/ folder into clear domains: - instrumentation/ for event-source hooks (dom, history, location, xhr) plus the PerformanceObserver layer, renamed from metrics/instrument.ts to instrumentation/performanceObserver.ts to stop colliding with the folder - web-vitals/ for web-vital tracking, spans, inp, lcp, reportEvents, and the browser helpers merged into a single utils.ts - performance/ for generic performance-entry enrichment: entries, element/ user/resource timing, and shared utils browserMetrics.ts is split into web-vitals/tracking.ts and performance/ entries.ts, and utils.ts into web-vitals/reportEvents.ts and performance/ utils.ts. Public API (index.ts re-exports) is unchanged; only internal paths and the test tree move.
logaretm
force-pushed
the
awad/webvitals-restructure
branch
from
August 5, 2026 19:44
7a246f5 to
12d0fc5
Compare
logaretm
marked this pull request as ready for review
August 5, 2026 20:05
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+71
to
+79
| export const onHidden = (cb: OnHiddenCallback) => { | ||
| const onHiddenCallback = (event: Event) => { | ||
| if (WINDOW.document?.visibilityState === 'hidden') { | ||
| cb(event); | ||
| } | ||
| }; | ||
|
|
||
| addPageListener('visibilitychange', onHiddenCallback, { capture: true }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Bug: The new onHidden function adds a visibilitychange event listener that is never removed, causing a minor memory leak each time it's called.
Severity: LOW
Suggested Fix
The onHidden function should return a function that, when called, removes the event listener it added. The caller, listenForWebVitalReportEvents, should then invoke this cleanup function after the web vital has been reported to ensure the listener is properly garbage collected. Alternatively, use the existing getVisibilityWatcher().onHidden method which already manages listener cleanup.
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/utils.ts#L71-L79
Potential issue: The new `onHidden` function adds a `visibilitychange` event listener
via `addPageListener` but never removes it. This function is called by
`listenForWebVitalReportEvents` for LCP and CLS tracking, resulting in new, persistent
event listeners being added on each call. While a guard (`_runCollectorCallbackOnce`)
prevents the callback from executing more than once, the listener object itself is never
garbage collected, leading to a memory leak. In a typical page load, this results in a
couple of unremoved listeners, which is a minor issue. However, in long-lived
single-page applications or if tracking functions are called multiple times, the
accumulation of listeners could become problematic.
Also affects:
packages/browser-utils/src/web-vitals/reportEvents.ts:34~36
Did we get this right? 👍 / 👎 to inform future reviews.
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
1 participant
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.
Reorganizes
browser-utils/srcaway from the catch-allmetrics/folder into clear domains:instrumentation/(dom/history/location/xhr + the PerformanceObserver layer),web-vitals/(tracking, spans, inp, lcp, reportEvents, and the helpers merged into one utils), andperformance/(entries, element/user/resource timing, shared utils).The reasoning is we will actually have some metric emitting logic in there and we don't want it to be confused with pre-existing logic like in #22397
Following up on pulling in
webvitalsas a dependency, I took this chance to clean up our structure and it does have some minor bundle-size improvements. The bundle increases are due to #23070 which explains where the increase is coming from and why we are willing to absorb it.