feat(llm): 🗒️ log content card impression when 50% of the card is shown #8526

thesan · 2024-11-28T15:36:40Z

✅ Checklist

npx changeset was attached.
Covered by automatic tests.
Impact of the changes:
- ...

📝 Description

Depending on the type of content card, use a mix of:

React Native: FlatList.onViewableItemsChanged
and a custom hook (which compare the element position to the screen position).

Demo:

Analytics log:

Screen.Recording.2024-11-29.at.18.02.02.mov

Make things clearer with a console log and the category id and the cards id sliced from the 6 to the 12 char:

Screen.Recording.2024-11-29.at.17.55.46.mov

❓ Context

LIVE-14465

🧐 Checklist for the PR Reviewers

The code aligns with the requirements described in the linked JIRA or GitHub issue.
The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
There are no undocumented trade-offs, technical debt, or maintainability issues.
The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
Any new dependencies have been justified and documented.
Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

vercel · 2024-11-28T15:36:44Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

4 Skipped Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	Dec 3, 2024 9:56am
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Dec 3, 2024 9:56am
react-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Dec 3, 2024 9:56am
web-tools	⬜️ Ignored (Inspect)	Visit Preview	Dec 3, 2024 9:56am

LucasWerey · 2024-11-29T09:02:14Z

.../ledger-live-mobile/src/newArch/features/DynamicContent/components/LogContentCardWrapper.tsx

+  const { logImpressionCard } = useDynamicContent();
+
+  useInViewContext(ref, ({ isInView }) => {
+    if (isInView) logImpressionCard(id);


We may also need to add normal analytics too here

LucasWerey · 2024-11-29T09:04:26Z

apps/ledger-live-mobile/src/screens/WalletCentricAsset/AssetDynamicContent.tsx

 import { Linking } from "react-native";
 import useDynamicContent from "~/dynamicContent/useDynamicContent";
+import LogContentCardWrapper from "~/newArch/features/DynamicContent/components/LogContentCardWrapper";


Suggested change

import LogContentCardWrapper from "~/newArch/features/DynamicContent/components/LogContentCardWrapper";

import LogContentCardWrapper from "LLM/features/DynamicContent/components/LogContentCardWrapper";

thesan · 2024-12-02T13:20:01Z

Demos for the other content cards

with console.log("[test-impression] logImpressionCard", cardId.slice(6, 12), card?.extras.title) added in logImpressionCard for clarity:

Notifications cards:

Screen.Recording.2024-12-02.at.12.37.31.mov

Wallet cards:

(Nb: When the 3rd card is swipped back the 2de card impression is not retriggered because it did not completely left the screen. However when this card is swipped back the 1st card impression is retriggered)

Screen.Recording.2024-12-02.at.10.36.53.mov

Asset cards:

Screen.Recording.2024-12-02.at.11.20.26.mov

Carousel category cards:

Screen.Recording.2024-12-02.at.14.15.53.mov

Uique category cards:

Screen.Recording.2024-12-02.at.14.17.11.mov

LucasWerey · 2024-12-02T16:36:53Z

apps/ledger-live-mobile/src/newArch/contexts/InViewContext/index.tsx

+  deps: DependencyList = [],
+) {
+  const { addWatchedItem, removeWatchedItem } = useContext(InViewContext);
+  const onInViewUpdateCb = useCallback(onInViewUpdate, deps); // eslint-disable-line react-hooks/exhaustive-deps


is the bypass really necessary ? We should avoid that

I might be missing another posibility but here's how I see it:

The way I see not to bypass the react-hooks/exhaustive-deps would be to go:
From version 1

// Declaration function useInViewContext( target: RefObject<View>, onInViewUpdate: (entry: InViewEntry) => void, deps: DependencyList = [], ) ... // Usage useInViewContext(target, ({ isInView }) => { if (isInView) logImpressionCard(id); }, [id]);

To version 2

// Declaration function useInViewContext( target: RefObject<View>, onInViewUpdate: (entry: InViewEntry) => void ) ... // Usage useInViewContext(target, useCallback(({ isInView }) => { if (isInView) logImpressionCard(id); }, [id]));

The problem I have with version 2 is that the type signature doesn't make it clear onInViewUpdate is a dependency which means writing this would be problematic:

useInViewContext(target, ({ isInView }) => { if (isInView) logImpressionCard(id); });

(because the internal useEffect will rerender on every re-render).

In react we are used to pass functions to hooks directly (without wrapping them in useCallback). So I think that here forwarding the dependencies to the hook is better to avoid mistakes. However it means that the variable used to forward the dependencies can't be statically analized by react-hooks/exhaustive-deps which makes the bypass necessary.

That said I could add useInViewContext to the config rules."react-hooks/exhaustive-deps".additionalHooks. So I'll have to add logImpressionCard to all the deps.

Done: d444580

LucasWerey · 2024-12-03T09:47:09Z

apps/ledger-live-mobile/src/components/Carousel/index.tsx

-        />
+        <LogContentCardWrapper key={cardProps.id + index} id={cardProps.id}>
+          <CarouselCard
+            key={cardProps.id + index}


I think you can remove this key as it's already used on the parent

…wn (#8526) * feat(llm): log category cards impression when 50% of the card is visible * feat(llm): do not run in view checks when no items is being watched * feat(llm): log portfolio content cards impression on 50% visibility * chore: update change log * feat(llm): log dynamic content card impression when 50% is shown * feat(llm): log notification impression when 50% is shown * chore(llm): rename IsInViewContext to InViewContext * chore(llm): add InViewContextProvider to the AppProviders * fix(llm): undefined order in null * feat(lld): log category impression when 50% is shown * chore(llm): use LLM alias in imports * feat(llm): trigger segment event on card impression * fix(llm): rewatch card when `LogContentCardWrapper.id` change * fix(llm): use mobileCardsSelector to find the card to log the impression * fix(llm): race condition between interval and inViewStatus * chore(llm): check "exhaustive-deps" on `useInViewContext` * chore(llm): remove unnecessary key

github-actions bot assigned thesan Nov 28, 2024

live-github-bot bot added the mobile Has changes in LLM label Nov 28, 2024

thesan force-pushed the feat/llm-enhanced-ccard-impression-tracking branch 2 times, most recently from 33c644d to 1747c22 Compare November 29, 2024 08:20

thesan marked this pull request as ready for review November 29, 2024 08:58

thesan requested a review from a team as a code owner November 29, 2024 08:58

LucasWerey reviewed Nov 29, 2024

View reviewed changes

thesan requested a review from LucasWerey November 29, 2024 17:07

thesan force-pushed the feat/llm-enhanced-ccard-impression-tracking branch from 1747c22 to 8761dc7 Compare November 29, 2024 17:08

thesan force-pushed the feat/llm-enhanced-ccard-impression-tracking branch from 8761dc7 to 45f1354 Compare December 2, 2024 13:21

LucasWerey reviewed Dec 2, 2024

View reviewed changes

thesan force-pushed the feat/llm-enhanced-ccard-impression-tracking branch from 45f1354 to d444580 Compare December 3, 2024 08:49

LucasWerey reviewed Dec 3, 2024

View reviewed changes

thesan added 15 commits December 3, 2024 10:55

feat(llm): log category cards impression when 50% of the card is visible

97f6f19

feat(llm): do not run in view checks when no items is being watched

7acd628

feat(llm): log portfolio content cards impression on 50% visibility

f0716d2

chore: update change log

889ad7d

feat(llm): log dynamic content card impression when 50% is shown

96d2c50

feat(llm): log notification impression when 50% is shown

01fd440

chore(llm): rename IsInViewContext to InViewContext

b74ffaa

chore(llm): add InViewContextProvider to the AppProviders

597e6d5

fix(llm): undefined order in null

5cd50ce

feat(lld): log category impression when 50% is shown

aa01cd3

chore(llm): use LLM alias in imports

c51dc00

feat(llm): trigger segment event on card impression

33362ff

fix(llm): rewatch card when LogContentCardWrapper.id change

21931e1

fix(llm): use mobileCardsSelector to find the card to log the impression

6a81407

fix(llm): race condition between interval and inViewStatus

f7551f0

thesan added 2 commits December 3, 2024 10:55

chore(llm): check "exhaustive-deps" on useInViewContext

9ac79e2

chore(llm): remove unnecessary key

7378513

thesan force-pushed the feat/llm-enhanced-ccard-impression-tracking branch from d444580 to 7378513 Compare December 3, 2024 09:55

LucasWerey approved these changes Dec 3, 2024

View reviewed changes

thesan merged commit fc50509 into develop Dec 3, 2024
41 checks passed

thesan deleted the feat/llm-enhanced-ccard-impression-tracking branch December 3, 2024 11:01

thesan mentioned this pull request Dec 3, 2024

feat(llm): ✨ log content card impression when 50% of the card is shown #8576

Merged

3 tasks

This was referenced Jan 7, 2025

[Coin-Module] Fix XRP pagination issue #8834

Merged

fix tezos pagination #8875

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(llm): 🗒️ log content card impression when 50% of the card is shown #8526

feat(llm): 🗒️ log content card impression when 50% of the card is shown #8526

thesan commented Nov 28, 2024 •

edited

Loading

vercel bot commented Nov 28, 2024 •

edited

Loading

LucasWerey Nov 29, 2024

thesan Nov 29, 2024

LucasWerey Nov 29, 2024

thesan commented Dec 2, 2024

LucasWerey Dec 2, 2024

thesan Dec 2, 2024 •

edited

Loading

thesan Dec 3, 2024

LucasWerey Dec 3, 2024

	import LogContentCardWrapper from "~/newArch/features/DynamicContent/components/LogContentCardWrapper";
	import LogContentCardWrapper from "LLM/features/DynamicContent/components/LogContentCardWrapper";

feat(llm): 🗒️ log content card impression when 50% of the card is shown #8526

feat(llm): 🗒️ log content card impression when 50% of the card is shown #8526

Conversation

thesan commented Nov 28, 2024 • edited Loading

✅ Checklist

📝 Description

Demo:

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented Nov 28, 2024 • edited Loading

LucasWerey Nov 29, 2024

Choose a reason for hiding this comment

thesan Nov 29, 2024

Choose a reason for hiding this comment

LucasWerey Nov 29, 2024

Choose a reason for hiding this comment

thesan commented Dec 2, 2024

Demos for the other content cards

Notifications cards:

Wallet cards:

Asset cards:

Carousel category cards:

Uique category cards:

LucasWerey Dec 2, 2024

Choose a reason for hiding this comment

thesan Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

thesan Dec 3, 2024

Choose a reason for hiding this comment

LucasWerey Dec 3, 2024

Choose a reason for hiding this comment

thesan commented Nov 28, 2024 •

edited

Loading

vercel bot commented Nov 28, 2024 •

edited

Loading

thesan Dec 2, 2024 •

edited

Loading