-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🔊 [RUMF-1577] Collect page lifecycle states #2229
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2229 +/- ##
==========================================
+ Coverage 94.01% 94.07% +0.06%
==========================================
Files 201 201
Lines 6083 6095 +12
Branches 1347 1349 +2
==========================================
+ Hits 5719 5734 +15
+ Misses 364 361 -3
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
978088b
to
a16526f
Compare
const limit = Math.max(0, pageStateEntries.length - maxPageStateEntriesSelectable) | ||
|
||
for (let index = pageStateEntries.length - 1; index >= limit; index--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥜 nitpick: It could be easier to grasp as:
for (let index = pageStateEntries.length - 1; index >= 0 && pageStateServerEntries.length < maxPageStateEntriesSelectable; index--) {
}
Or, maybe you could adapt the previous version as: .findAll().slice(0, maxPageStateEntriesSelectable).reverse().map()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, I was hesitating to do that in the first place but as it is less effective performance wise I went with a good old loop. I don't have a strong opinion though. Maybe a third party @bcaudan can help deciding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought, maybe adding a couple comments could make this code easier to grasp while keeping the most performant approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments:
browser-sdk/packages/rum-core/src/domain/contexts/pageStateHistory.ts
Lines 84 to 88 in 6f8127f
// limit the number of entries to return | |
const limit = Math.max(0, pageStateEntries.length - maxPageStateEntriesSelectable) | |
// loop page state entries backward to return the selected ones in desc order | |
for (let index = pageStateEntries.length - 1; index >= limit; index--) { |
Motivation
Modern browsers today will sometimes suspend pages or discard them entirely when system resources are constrained, which can lead to unexpected behaviors. The Page Lifecycle API, provides lifecycle hooks so we can detect and handle these browser interventions.
This PR, collect page lifecycle state changes during a view to help Browser SDK issue investigations.
Changes
Testing
I have gone over the contributing documentation.