Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/app/Components/Countdown/Ticker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ export const durationSections = (duration: Duration, labels: [string, string, st
label: labels[0],
},
{
time: padWithZero(d.hours),
time: padWithZero(Math.floor(d.hours)),
label: labels[1],
},
{
time: padWithZero(d.minutes),
time: padWithZero(Math.floor(d.minutes)),
label: labels[2],
},
{
time: padWithZero(d.seconds),
time: padWithZero(Math.floor(d.seconds)),
Comment thread
github-actions[bot] marked this conversation as resolved.
label: labels[3],
},
]
Expand Down
12 changes: 12 additions & 0 deletions src/app/Components/Countdown/__tests__/Ticker.tests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ describe("SimpleTicker", () => {
})
).toBeTruthy()
})

it("does not render fractional seconds / milliseconds", () => {
// 1s + 789ms — the fractional remainder that used to leak through
const fractional = Duration.fromMillis(1789)
renderWithWrappers(<SimpleTicker duration={fractional} separator=" " variant="sm-display" />)

expect(
screen.getByText("00d 00h 00m 01s", {
normalizer: getDefaultNormalizer({ collapseWhitespace: false }),
})
).toBeTruthy()
})
})

describe("LabeledTicker", () => {
Expand Down
84 changes: 48 additions & 36 deletions src/app/Scenes/HomeView/Sections/HomeViewSectionArtworks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { NoFallback, withSuspense } from "app/utils/hooks/withSuspense"
import { isDislikeArtworksEnabledFor } from "app/utils/isDislikeArtworksEnabledFor"
import { useMemoizedRandom } from "app/utils/placeholders"
import { times } from "lodash"
import { memo, useEffect, useRef } from "react"
import { memo, useEffect, useState } from "react"
import { fetchQuery, graphql, useFragment, useLazyLoadQuery } from "react-relay"

interface HomeViewSectionArtworksProps extends FlexProps {
Expand Down Expand Up @@ -64,49 +64,59 @@ export const HomeViewSectionArtworks: React.FC<HomeViewSectionArtworksProps> = (

const isRailInViewport = viewableSections.includes(section.internalID)

// Ref so the async `complete` callback reads current visibility, not the value at kick-off.
const isRailInViewportRef = useRef(isRailInViewport)
isRailInViewportRef.current = isRailInViewport
// Set when a live refresh completes; cleared once we've re-fired railViewed for it. Lets us fire
// when the rail is on screen — now, or later when it's scrolled into view.
const [hasPendingRailViewed, setHasPendingRailViewed] = useState(false)

const { onViewableItemsChanged, viewabilityConfig, resetTracking } = useItemsImpressionsTracking({
// It is important here to tell if the rail is visible or not, because the viewability config
// default behavior, doesn't take into account the fact that the rail could be not visible
// on the screen because it's within a scrollable container.
isInViewport: isRailInViewport && section.trackItemImpressions,
contextModule,
})
const { onViewableItemsChanged, viewabilityConfig, resetTracking, trackingKey } =
useItemsImpressionsTracking({
// It is important here to tell if the rail is visible or not, because the viewability config
// default behavior, doesn't take into account the fact that the rail could be not visible
// on the screen because it's within a scrollable container.
isInViewport: isRailInViewport && section.trackItemImpressions,
contextModule,
})

// On a refetch bump, force-fetch this rail. This effect runs per-section, so every live rail
// (recommended, new works for you, and any future ones in liveSectionIDs) refetches — in view
// or not — so no live rail goes stale. On complete, reset the impression guard and re-fire
// railViewed only if the rail is on screen.
// On a refetch bump, flag that railViewed should re-fire for this refresh (the effect below
// fires it once the rail is on screen — now or when scrolled to), and force-fetch this rail. This
// effect runs per-section, so every live rail (recommended, new works for you, and any future
// ones in liveSectionIDs) refetches — in view or not — so no live rail goes stale. On complete we
// reset the per-item impression guard so items re-track against the fresh data.
useEffect(() => {
if (!isLiveRefreshRail || liveRefetchKey === 0) {
return
}
if (isLiveRefreshRail && liveRefetchKey > 0) {
setHasPendingRailViewed(true)

const subscription = fetchQuery<HomeViewSectionArtworksQuery>(
getRelayEnvironment(),
homeViewSectionArtworksQuery,
{ id: section.internalID, enableHidingDislikedArtworks },
{ networkCacheConfig: { force: true } }
).subscribe({
complete: () => {
resetTracking()

if (isRailInViewportRef.current) {
tracking.viewedSection(contextModule, index)
}
},
error: (error: Error) => {
console.error("Failed to refresh live artworks rail", error)
},
})
const subscription = fetchQuery<HomeViewSectionArtworksQuery>(
getRelayEnvironment(),
homeViewSectionArtworksQuery,
{ id: section.internalID, enableHidingDislikedArtworks },
{ networkCacheConfig: { force: true } }
).subscribe({
complete: () => {
resetTracking()
},
error: (error: Error) => {
console.error("Failed to refresh live artworks rail", error)
},
})

return () => subscription.unsubscribe()
}

return () => subscription.unsubscribe()
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [liveRefetchKey])

// Re-fire railViewed after a live refresh, once the rail is on screen. Driven by the reactive
// `isRailInViewport` (from viewableSections, which updates on scroll), so a rail that was off
// screen at refresh time still re-fires when it's later scrolled into view.
useEffect(() => {
if (hasPendingRailViewed && isRailInViewport) {
tracking.viewedSection(contextModule, index)
setHasPendingRailViewed(false)
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [hasPendingRailViewed, isRailInViewport])
Comment on lines +112 to +118

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.

Two things about this effect:

1. Duplicate railViewed on first scroll-into-view. HomeViewSectionSentinel (rendered at line 216) also fires viewedSection the first time the section becomes visible — its only guard is trackedSections (HomeViewSectionSentinel.tsx:27), which is per-contextModule, not per-refresh. Scenario:

  1. Cold home view, the recommended/NWFY rail is below the fold → sentinel hasn't fired, viewableSections doesn't include the section.
  2. User navigates away and back → useFocusEffect in HomeView.tsx:172 bumps liveRefetchKeyhasPendingRailViewed = true.
  3. User scrolls the rail into view → the outer FlashList sets viewableSections and the sentinel flips visible.

Both paths now call tracking.viewedSection(contextModule, index) → two railViewed events for the same rail in the same moment.

2. railViewed fires before the refreshed data lands. setHasPendingRailViewed(true) runs synchronously on the bump, so when the rail is already on screen this effect fires in the same commit — before fetchQuery resolves, and it fires even if the fetch errors. The old code fired from complete. If that's the intended behaviour, worth saying so in the comment, since "re-fire railViewed after a live refresh" reads as "after the refresh landed".


let artworks = extractNodes(section.artworksConnection)

if (isDislikeArtworksEnabledFor(contextModule)) {
Expand Down Expand Up @@ -182,6 +192,8 @@ export const HomeViewSectionArtworks: React.FC<HomeViewSectionArtworksProps> = (
onArtworkPress={handleOnGridArtworkPress}
trackItemImpressions={section.trackItemImpressions}
contextModule={contextModule}
// We are using this key to force a re-render to track item impressions again
key={trackingKey}
Comment on lines +195 to +196

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 comment says "force a re-render", but a changing key forces a full unmount + remount of HomeViewSectionArtworksGrid, which throws away its local state. That resets hasGridLaidOut to false (HomeViewSectionArtworksGrid.tsx:34), so the "View More" button unmounts and only comes back after the async measureInWindow callback lands (HomeViewSectionArtworksGrid.tsx:40-46). Every live refresh of a grid section flashes that CTA out and back in, and remounts all ArtworkGridItems with it.

The useLayoutEffect already re-measures on [artworks], so the remount isn't needed for layout — it's only there to clear the grid's own trackedGridItems ref (line 38), which the parent's resetTracking() doesn't touch. Clearing that set directly is cheaper and avoids the flash, e.g. pass trackingKey down and:

useEffect(() => {
  trackedGridItems.clear()
}, [trackingKey])

/>
) : (
<ArtworkRail
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -425,8 +425,8 @@ describe("HomeViewSectionArtworks", () => {
setLiveSectionIDs([])
})

it("re-fires railViewed only after the live refresh completes", () => {
const { env } = renderWithRelay({
it("re-fires railViewed as soon as the rail is refreshed", () => {
renderWithRelay({
HomeViewSectionArtworks: () => RECOMMENDED_SECTION,
})

Expand All @@ -437,20 +437,6 @@ describe("HomeViewSectionArtworks", () => {
homeViewStoreActions.bumpLiveRefetchKey()
})

// Nothing fires yet — the refreshed data hasn't landed.
expect(mockTrackEvent).not.toHaveBeenCalledWith(
expect.objectContaining({ action: "railViewed" })
)

// Once the forced refetch completes, railViewed fires for the fresh data.
act(() => {
env.mock.resolveMostRecentOperation((operation) =>
MockPayloadGenerator.generate(operation, {
HomeViewSectionArtworks: () => RECOMMENDED_SECTION,
})
)
})

expect(mockTrackEvent).toHaveBeenCalledWith({
action: "railViewed",
context_module: "newWorksForYouRail",
Expand All @@ -459,8 +445,7 @@ describe("HomeViewSectionArtworks", () => {
})
})

it("does not re-fire railViewed on refresh when the WTYL rail is off screen", () => {
// The refresh-driven railViewed re-fire only happens while the rail is actually on screen.
it("re-fires railViewed once an off-screen rail is scrolled into view after a refresh", () => {
const { env } = renderWithRelay({
HomeViewSectionArtworks: () => RECOMMENDED_SECTION,
})
Expand All @@ -480,10 +465,17 @@ describe("HomeViewSectionArtworks", () => {
)
})

// railViewed should not fire because the rail is off screen.
// Off screen at refresh time — nothing fires yet.
expect(mockTrackEvent).not.toHaveBeenCalledWith(
expect.objectContaining({ action: "railViewed" })
)

// Scrolled into view afterwards — the pending refresh re-fires railViewed.
act(() => {
homeViewStoreActions.setViewableSections(["home-view-section-recommended-artworks"])
})

expect(mockTrackEvent).toHaveBeenCalledWith(expect.objectContaining({ action: "railViewed" }))
})

it("re-fires railViewed on every refresh while the rail is in view", () => {
Expand Down Expand Up @@ -655,8 +647,8 @@ describe("HomeViewSectionArtworks", () => {
setLiveSectionIDs([])
})

it("re-fires railViewed only after the live refresh completes", () => {
const { env } = renderWithRelay({
it("re-fires railViewed as soon as the rail is refreshed", () => {
renderWithRelay({
HomeViewSectionArtworks: () => NWFY_SECTION,
})

Expand All @@ -667,19 +659,6 @@ describe("HomeViewSectionArtworks", () => {
homeViewStoreActions.bumpLiveRefetchKey()
})

// Nothing fires yet — the refreshed data hasn't landed.
expect(mockTrackEvent).not.toHaveBeenCalledWith(
expect.objectContaining({ action: "railViewed" })
)

act(() => {
env.mock.resolveMostRecentOperation((operation) =>
MockPayloadGenerator.generate(operation, {
HomeViewSectionArtworks: () => NWFY_SECTION,
})
)
})

expect(mockTrackEvent).toHaveBeenCalledWith({
action: "railViewed",
context_module: "newWorksForYouRail",
Expand Down
9 changes: 6 additions & 3 deletions src/app/Scenes/HomeView/hooks/useImpressionsTracking.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const useItemsImpressionsTracking = ({
// Use different state management for test vs production
const renderedItems = useSharedValue<Array<TrackableItem>>([])
const [testRenderedItems, setTestRenderedItems] = useState<Array<TrackableItem>>([])
const [key, setKey] = useState<number>(0)

const trackedItems = useRef<Set<string>>(new Set()).current

Expand Down Expand Up @@ -106,18 +107,20 @@ export const useItemsImpressionsTracking = ({
runOnJS(trackItems)(currentItems)
}
},
[enableItemsViewsTracking, isInViewport, contextScreenOwnerType, contextModule]
[enableItemsViewsTracking, isInViewport, contextScreenOwnerType, contextModule, key]

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.

key is added to the useAnimatedReaction deps (production path) but not to the __TEST__ effect at lines 89-100. Nothing in that dep list changes when key bumps — trackItems deps (lines 57-65) don't include key either — so in tests a resetTracking() never re-triggers trackItems on its own.

That means the behaviour this dep buys is untested: HomeViewSectionArtworks.tests.tsx:512 ("re-enables itemViewed tracking after the live refresh completes") passes only because the test manually re-invokes onViewableItemsChanged after the refresh. The real case for the rail path — rail stays exactly in place after returning to home, so viewable items never change — is what the key dep is for and it isn't covered. Adding key to the test effect's deps would make the two paths behave the same and let a test assert it.

)

// Clears the per-item "already tracked" guard so itemViewed events become
// eligible to fire again, e.g. after a forced data refresh of the rail.
const resetTracking = useCallback(() => {
trackedItems.clear()
}, [trackedItems])
setKey(key + 1)
}, [trackedItems, key])
Comment on lines 115 to +118

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.

setKey(key + 1) reads key from the closure, and resetTracking is invoked from a long-lived async callback (fetchQuery(...).subscribe({ complete }) in HomeViewSectionArtworks.tsx:95) whose enclosing effect deps are just [liveRefetchKey]. It happens to work today because the effect re-runs on every bump, but a functional update removes the staleness question and stops resetTracking's identity from churning on every reset:

Suggested change
const resetTracking = useCallback(() => {
trackedItems.clear()
}, [trackedItems])
setKey(key + 1)
}, [trackedItems, key])
const resetTracking = useCallback(() => {
trackedItems.clear()
setKey((prev) => prev + 1)
}, [trackedItems])


return {
onViewableItemsChanged,
viewabilityConfig,
resetTracking,
trackingKey: key,
viewabilityConfig,
}
}
2 changes: 1 addition & 1 deletion src/app/Scenes/MyProfile/MyProfileEditForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ export const MyProfileEditForm: React.FC<MyProfileEditFormProps> = () => {
<Message
variant="info"
title="Complete your profile and make a great impression"
text="The information you provide here will be shared when you contact a gallery or make an offer."
text="The information you provide here will be shared when you contact a gallery, start an order, set an alert, or save an artwork."
showCloseButton
/>
)}
Expand Down
Loading