Skip to content

[WIP] feat(ONYX-2187): fix railViewed/itemViewed tracking gaps on live home rails - #13819

Open
dariakoko wants to merge 8 commits into
mainfrom
ONYX-2187/address-impression-tracking-issues
Open

[WIP] feat(ONYX-2187): fix railViewed/itemViewed tracking gaps on live home rails #13819
dariakoko wants to merge 8 commits into
mainfrom
ONYX-2187/address-impression-tracking-issues

Conversation

@dariakoko

@dariakoko dariakoko commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

This PR resolves ONYX-2187

Description

Summary
WTYL and NWFY refetch their data after user interaction (returning to Home, pull-to-refresh). Two tracking gaps existed around that refresh:

  • rail_viewed wasn't re-firing after a live refresh.
  • item_viewed wasn't re-firing for NWFY grid artworks after a live refresh.

Root cause for both: the shared Sentinel component's polling effect has an empty dependency array, so its visibility comparison freezes at mount — it can only ever report one true transition per mount, never again. That's invisible for its many other consumers (they only care about the first appearance), but breaks any rail that needs to re-detect "still/again on screen" after a refresh.

Changes
HomeViewSectionSentinel: for live rails only (isLiveRefreshRail), replaces Sentinel with a locally-owned, ref-based visibility poll, plus an explicit re-check tied to refresh completion. Every other section (~15 types) keeps rendering Sentinel unchanged.
useGridItemVisibility (new hook): same fix, scoped to NWFY's grid items.
GridItemVisibilitySentinel (new): same fix, scoped to NWFY's grid items.
HomeViewContext.ts: removeTrackedSection now no-ops when there's nothing to remove, avoiding an unrelated re-render cascade across every mounted home section on each refresh.

Why useGridItemVisibility is a hook, not a component
It's implemented as a hook rather than a component so it doesn't add any node to the render tree — the caller attaches it directly to its existing card's own ref. A component that wraps the card in an extra View (to have something real to measure) interferes with FlashList's masonry column-height measurement and can push later grid items off screen.

It's opted into via a stable useLiveVisibilityTracking boolean threaded through MasonryArtworkGridItemMasonryInfiniteScrollArtworkGridGenericGrid — stable because it never changes for a given grid instance. A per-refresh counter would force FlashList's renderItem identity to change (and the whole grid to re-render/re-layout) on every refresh; a static boolean can't. The other 25+ callers of this shared grid stack never pass it and keep using Sentinel unchanged.


Impressions tracking on NWFY (tracks after refresh on back, tracks after pull to refresh)

nwfy.tracking.ready.mov

Impressions tracking on WTYL (tracks after refresh on back, tracks after pull to refresh)

wtyl.tracking.ready.mov

PR Checklist

  • I have tested my changes on the following platforms:
    • Android.
    • iOS.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos at least on Android, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • fix railViewed/itemViewed tracking gaps on live home rails -daira

Need help with something? Have a look at our docs, or get in touch with us.

@dariakoko dariakoko self-assigned this Jul 23, 2026
@github-actions github-actions Bot deleted a comment from claude Bot Jul 23, 2026
Comment thread src/app/Components/ArtworkGrids/useGridItemVisibility.ts Outdated
Comment thread src/app/Components/ArtworkGrids/useGridItemVisibility.ts Outdated
@github-actions github-actions Bot deleted a comment from claude Bot Jul 23, 2026
Comment thread src/app/Components/ArtworkGrids/useGridItemVisibility.ts
@artsyit

artsyit commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

This PR contains the following changes:

  • Dev changes (fix railViewed/itemViewed tracking gaps on live home rails -daira - dariakoko)

Generated by 🚫 dangerJS against aef72b6

@github-actions github-actions Bot deleted a comment from claude Bot Jul 24, 2026
Comment on lines +59 to +61
// requestAnimationFrame is mocked as `setTimeout(cb, 0)` in the RN jest environment, so the
// refresh re-check effect's measurement runs on a real (if immediate) timer tick.
const flushRaf = () => act(() => new Promise((resolve) => setTimeout(resolve, 0)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment says the re-check runs via requestAnimationFrame, but neither HomeViewSectionSentinel nor useGridItemVisibility uses rAF — both re-check via a 1000ms setInterval poll. The flushRaf helper name and comment are misleading. Rename to reflect that it flushes the timer/microtask queue, or drop the rAF reference.

Comment on lines +47 to +53
useEffect(() => {
if (!refreshKey) {
return
}

trackedGridItems.clear()
}, [refreshKey, trackedGridItems])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refreshKeytrackedGridItems.clear() path is the core NWFY item_viewed re-fire fix, but it has no test. HomeViewSectionArtworksGrid.tests.tsx still only covers the once-per-id case and wasn't touched in this PR. Add a test that fires a visibility change, bumps refreshKey, then fires again and asserts item_viewed re-fires for the same artwork id.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Code Review

Summary

Fixes two tracking gaps on the live-refreshing home rails (WTYL, NWFY): rail_viewed and item_viewed didn't re-fire after a live refresh. Root cause is the shared Sentinel's polling effect having an empty dependency array, so its comparison value freezes at mount and it can only ever report one true transition per lifetime. The PR sidesteps that for the live-rail case with a locally-owned, ref-based poll (HomeViewSectionSentinel for rails, the new useGridItemVisibility hook for grid items), threads a stable useLiveVisibilityTracking boolean down the grid stack, and resets the section/item guards on refresh completion. Every other consumer keeps Sentinel unchanged.

I verified the core Sentinel claim: isInViewPort closes over isVisible captured at mount (useEffect(..., [])), so once it flips false→true it never compares against anything but the stale false and never re-fires. The fix is well-scoped and the reasoning (hook over wrapping View, to avoid perturbing FlashList masonry measurement) is sound.

Issues Found

🟡 Important — test gap on the NWFY item_viewed re-fire
The refreshKeytrackedGridItems.clear() path in HomeViewSectionArtworksGrid.tsx is the main NWFY fix, but it has no test. HomeViewSectionArtworksGrid.tests.tsx still only covers the once-per-id case and wasn't updated. The new useGridItemVisibility hook is also untested. The railViewed tests were updated, but the item side (arguably the harder half) relies on manual QA only. (inline)

🟢 Suggestion — misleading test comment / helper name
HomeViewSectionArtworks.tests.tsx:59-61 says the re-check runs on a requestAnimationFrame tick, but both HomeViewSectionSentinel and useGridItemVisibility re-check via a 1000ms setInterval, not rAF. flushRaf is a misleading name. (inline)

🟢 Suggestion — stale PR description
The description says a GridItemVisibilitySentinel component was added; the diff only adds the useGridItemVisibility hook (the component approach was dropped, as the hook doc correctly explains). Worth updating so reviewers aren't hunting for a file that isn't there.

🟢 Suggestion — duplicated polling logic
HomeViewSectionSentinel's inline poll and useGridItemVisibility are near-identical (ref + setInterval + measure + focus-pause). Different viewport math (rail = any overlap; item = 50% within content area minus tab bar), so not a blocker, but a shared primitive would cut the duplication.

Areas Reviewed

  • Bugs & edge cases: The refresh flow is correct — removeTrackedSection(contextModule) resets the once-ever guard and setLiveRefetchCompletionKey clears the per-item guard; handleVisibilityChange is rebuilt with fresh trackedSections and stored in a ref each render, so the next poll tick re-fires within ~1s. removeTrackedSection no-ops when absent, avoiding the re-render cascade. Not resetting trackedSectionTypes is fine — the fire gate is trackedSections, not the types set.
  • Performance: Every visible NWFY grid item runs its own 1000ms setInterval + native measure(). Polling pauses when Home loses focus (useIsFocused), which caps the cost, and FlashList only mounts on-screen items. Acceptable, matches the pre-existing Sentinel cost model.
  • Conventions: hooks over HOCs, no raw Keyboard import, no FlatList/nested scroll, no cross-scene imports (BOTTOM_TABS_HEIGHT is from app/Navigation, not a scene), no new barrel files. All good.

Note for the author

This is a tracking/visibility change with device behavior that's hard to unit-test end to end — the PR does include before/after videos on both platforms, thanks for that. Main ask is closing the item_viewed-on-refresh test gap.

@dariakoko dariakoko changed the title feat(ONYX-2187): fix railViewed/itemViewed tracking gaps on live home rails [WIP] feat(ONYX-2187): fix railViewed/itemViewed tracking gaps on live home rails Jul 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi there! 👋
We use Conventional Commit formatting for PR titles, but your PR title does not appear to follow this.
Please update your title to follow Conventional Commits guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants