-
Notifications
You must be signed in to change notification settings - Fork 357
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
feat(llm): ποΈ log content card impression when 50% of the card is shown #8526
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
97f6f19
feat(llm): log category cards impression when 50% of the card is visible
thesan 7acd628
feat(llm): do not run in view checks when no items is being watched
thesan f0716d2
feat(llm): log portfolio content cards impression on 50% visibility
thesan 889ad7d
chore: update change log
thesan 96d2c50
feat(llm): log dynamic content card impression when 50% is shown
thesan 01fd440
feat(llm): log notification impression when 50% is shown
thesan b74ffaa
chore(llm): rename IsInViewContext to InViewContext
thesan 597e6d5
chore(llm): add InViewContextProvider to the AppProviders
thesan 5cd50ce
fix(llm): undefined order in null
thesan aa01cd3
feat(lld): log category impression when 50% is shown
thesan c51dc00
chore(llm): use LLM alias in imports
thesan 33362ff
feat(llm): trigger segment event on card impression
thesan 21931e1
fix(llm): rewatch card when `LogContentCardWrapper.id` change
thesan 6a81407
fix(llm): use mobileCardsSelector to find the card to log the impression
thesan f7551f0
fix(llm): race condition between interval and inViewStatus
thesan 9ac79e2
chore(llm): check "exhaustive-deps" on `useInViewContext`
thesan 7378513
chore(llm): remove unnecessary key
thesan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"live-mobile": minor | ||
--- | ||
|
||
Log content card impression when 50% of the card is shown |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
92 changes: 92 additions & 0 deletions
92
apps/ledger-live-mobile/src/newArch/contexts/InViewContext/index.tsx
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
import React, { | ||
createContext, | ||
DependencyList, | ||
type ReactNode, | ||
type RefObject, | ||
useCallback, | ||
useContext, | ||
useEffect, | ||
useRef, | ||
useState, | ||
} from "react"; | ||
import { Dimensions, type View } from "react-native"; | ||
import { concatMap, from, interval } from "rxjs"; | ||
import type { InViewOptions, InViewContext, InViewEntry, WatchedItem } from "./types"; | ||
import { inViewStatus } from "./utils"; | ||
|
||
const InViewContext = createContext<InViewContext>({}); | ||
|
||
export function useInViewContext( | ||
onInViewUpdate: (entry: InViewEntry) => void, | ||
deps: DependencyList = [], | ||
target: RefObject<View>, | ||
) { | ||
const { addWatchedItem, removeWatchedItem } = useContext(InViewContext); | ||
const onInViewUpdateCb = useCallback(onInViewUpdate, deps); // eslint-disable-line react-hooks/exhaustive-deps | ||
|
||
useEffect(() => { | ||
const item = { target, onInViewUpdate: onInViewUpdateCb }; | ||
addWatchedItem?.(item); | ||
return () => removeWatchedItem?.(item); | ||
}, [target, onInViewUpdateCb, addWatchedItem, removeWatchedItem]); | ||
} | ||
|
||
export function InViewContextProvider({ | ||
inViewThreshold = 0.5, | ||
outOfViewThreshold = 0, | ||
interval: intervalDuration = 200, | ||
children, | ||
}: InViewOptions & { children: ReactNode }) { | ||
const items = useRef<WatchedItem[]>([]); | ||
const [hasItems, setHasItems] = useState(false); | ||
|
||
const addWatchedItem = useCallback((item: WatchedItem) => { | ||
if (items.current.length === 0) setHasItems(true); | ||
items.current.push(item); | ||
}, []); | ||
const removeWatchedItem = useCallback((item: WatchedItem) => { | ||
const index = items.current.indexOf(item); | ||
if (index === -1) return; | ||
items.current.splice(index, 1); | ||
if (items.current.length === 0) setHasItems(false); | ||
}, []); | ||
|
||
const watchedItem = useRef(new WeakMap<WatchedItem, boolean>()); | ||
|
||
useEffect(() => { | ||
if (!hasItems) return; | ||
|
||
const window = Dimensions.get("window"); | ||
const observer = interval(intervalDuration).pipe( | ||
concatMap(() => | ||
from( | ||
Promise.all( | ||
items.current.map(async item => { | ||
const threshold = watchedItem.current.get(item) | ||
? outOfViewThreshold | ||
: inViewThreshold; | ||
|
||
const entry = await inViewStatus(item.target, threshold, window); | ||
return { item, entry }; | ||
}), | ||
), | ||
), | ||
), | ||
); | ||
|
||
const subscription = observer.subscribe(res => { | ||
res.forEach(({ item, entry }) => { | ||
if (entry.isInView === watchedItem.current.get(item)) return; | ||
watchedItem.current.set(item, entry.isInView); | ||
item.onInViewUpdate(entry); | ||
}); | ||
}); | ||
return () => subscription.unsubscribe(); | ||
}, [hasItems, inViewThreshold, outOfViewThreshold, intervalDuration]); | ||
|
||
return ( | ||
<InViewContext.Provider value={{ addWatchedItem, removeWatchedItem }}> | ||
{children} | ||
</InViewContext.Provider> | ||
); | ||
} |
Oops, something went wrong.
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.
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.
is the bypass really necessary ? We should avoid that
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 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
To version 2
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:(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 byreact-hooks/exhaustive-deps
which makes the bypass necessary.That said I could add
useInViewContext
to the configrules."react-hooks/exhaustive-deps".additionalHooks
. So I'll have to addlogImpressionCard
to all the deps.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.
Done: d444580