Scroll depth tracker for home page with refactor#14071
Conversation
Co-authored-by: Copilot <copilot@github.com>
…thub.com:bbc/simorgh into scroll-depth-tracker-for-home-page-with-refactor
…er-for-home-page-with-refactor
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…er-for-home-page-with-refactor
…er-for-home-page-with-refactor
0ebe976 to
68442f2
Compare
…er-for-home-page-with-refactor
There was a problem hiding this comment.
Pull request overview
Refactors scroll depth tracking into a reusable hook that supports different “trackable content” bounds per page type, and integrates it on the Home Page while addressing a Reverb beacon race involving window.bbcpage.
Changes:
- Extend
useScrollDepthTrackerto accept a page-specific bounds function and addgetHomePageBounds(header→footer tracking). - Integrate scroll depth tracking into
HomePageby attaching the returned ref callback to<main>. - Prevent
sendBeaconfrom overwritingwindow.bbcpagefor non-pageview (component) events to avoid corrupting concurrent page view metadata.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/app/pages/HomePage/index.test.tsx | Mocks scroll depth hook to keep existing viewability call-count assertions stable under React StrictMode. |
| src/app/pages/HomePage/HomePage.tsx | Wires scroll depth tracking into the Home Page <main> via a ref callback and home-specific bounds. |
| src/app/lib/analyticsUtils/sendBeacon/index.ts | Only sets window.bbcpage for pageView events to avoid overwriting page-level metadata for component events. |
| src/app/hooks/useScrollDepthTracker/index.tsx | Adds configurable bounds (GetTrackingBounds), introduces home/article bounds helpers, and computes depth using cached bounds. |
| src/app/hooks/useScrollDepthTracker/index.test.tsx | Adds coverage for home page bounds behavior (header/footer tracking and fallbacks). |
| return () => window.removeEventListener('scroll', handleScroll); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- getBounds is intentionally omitted: it is called once at effect setup to cache the bounds, not on every scroll frame | ||
| }, [ | ||
| campaignID, | ||
| componentName, |
| // Measure the trackable boundaries once when tracking begins. Page landmarks | ||
| // (header, footer, hero image) don't move while the user scrolls, so there | ||
| // is no need to re-measure on every animation frame. | ||
| const { startY, endY } = getBounds(mainElement); |
There was a problem hiding this comment.
Home pages can still change height after this point when ads, images, fonts, banners, or embeds load. If the footer moves, the 75% and 100% events can fire too early or too late so bounds might need updated after layout changes
There was a problem hiding this comment.
I thought we had the ad placeholder box to stop layout shift, and that we would have stopped content shift on other things because it is bad for performance, but will check
| // setReverbPageValues with that minimal params.page would overwrite | ||
| // window.bbcpage and corrupt any concurrent page view beacon that has | ||
| // not yet been read by the Reverb SDK. | ||
| if (eventDetails.eventName === 'pageView') { |
There was a problem hiding this comment.
Code comments make sense re: window.bbcpage. If i understand it correctly, because this changes every component click/view event, not just scroll-depth, might be worth adding a test for regression showing that an early scroll-depth event does not break the page-view payload and still sends correctly via Reverb
There was a problem hiding this comment.
yeah this whole thing scares me
Refactors scroll depth tracker to allow use with different page types.
This pull request introduces a reusable scroll depth tracking hook and integrates it into both the Article and Home pages. The main goal is to enable analytics events when users scroll through key portions of these pages, supporting more accurate engagement measurement. The hook is flexible, allowing for different tracking bounds depending on the page type.
Integration with Home Page:
getHomePageBounds) inHomePage.tsx, initialized the tracker for the home page, and attached it to the<main>element. [1] [2] [[3]](diffhunk://#diff-a38554e294af0ab522c881a0c0ea4f833dcd781ed7d07fc5d0949676fba57a4fL83-R92on home page:
the bounds are measured once on set up rather than on every scroll event since the page landmarks (header, footer, hero image) dont' move or change whle the user scrolls.
Each page passes its own rule
`// ArticlePage — no third argument, uses the default
useScrollDepthTracker('article-scroll-depth', scrollDepthEnabled);
// HomePage — passes the home page rule explicitly
useScrollDepthTracker('homepage-scroll-depth', true, getHomePageBounds);`
EXTRA INFO ABOUT sendBeacon CHANGE:
Vefore reverb can send a page view it needs to know page-level metadata (including page name, destination, producer and x8 which identifies it as simorgh). Simorgh sets this by writing a global object cakked window.bbcpage.
When reverb.viewEvent() is called, the Reverb SDK reads window.bbcpage at that moment and does not cache it at startup. The reverb event hierarchy isL
There are 2 different functions that build the ReverbBeaconConfig object passed to sendBeacon:
Before this sendBeacon change, sendBeacon always called sendReverbPageValues for every event type. But component events don't need fresh page metadata, they are gated behind a promise that reverb has already read window.bbcpage, and it is not necessary to call setReverbPageValues with the minimal params.page????
There was a race condition introduced by the scroll depth tracker, which fires on the first requestAnimationFrame (very early), where page view fired, setReverbPageValues sets windoe.bbcpage with (x8 simorgh), callReverb registers a .then() on window.__reverb.__reverbLoadedPromise. Then scroll depth fires very soon after and overwrites the window.bbcpage with the minimal page.params instead. When the cypress query intercepts, it tries to match on queru: { x8 'simorgh' } but this is no longer there and the test fails.
The fix only uses setReverbPageValues for page view. Component events didn't need to use this (??) because page views set up the page-level data and component events gate on pageSentPromise and read the captured data (this.pageValues).
Summary
A very high-level summary of easily-reproducible changes that can be understood by non-devs, and why these changes where made.
Code changes
Testing
Useful Links