With tracking use callback

[dkonieczek]

No description provided.

[Copilot]

Pull request overview

This pull request refactors the impression tracking mechanism from using a resetKey prop to a callback-based approach using updateRef. The changes eliminate the need to serialize controller state by instead relying on responseId changes to detect when tracking should be reset.

Changes:

• Replaced resetKey parameter with updateRef callback in useIntersectionAdvanced hook, using a counter mechanism to trigger observer resets
• Modified impression tracking in withTracking to use responseId for identity tracking instead of serializing controller state
• Updated AutocompleteController to include filters in the search key for better impression deduplication

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/snap-preact-components/src/hooks/useIntersectionAdvanced.tsx	Removed resetKey parameter and added updateRef callback with counter-based reset mechanism; changed return value to object with inViewport and updateRef properties
packages/snap-preact-components/src/utilities/createImpressionObserver.ts	Updated to expose updateRef from useIntersectionAdvanced and pass it through
packages/snap-preact-components/src/providers/withTracking.tsx	Refactored to use responseId for tracking identity changes; added awaitingReobservationRef logic to prevent impressions during observer reset; created trackingRef callback for ref attachment
packages/snap-preact-demo/src/components/Results/Results.tsx	Updated to destructure inViewport from useIntersectionAdvanced return object due to API change
packages/snap-controller/src/Autocomplete/AutocompleteController.ts	Renamed lastSearchQuery to lastSearchKey and included filters in the key calculation for more accurate duplicate detection

Comments suppressed due to low confidence (2)

packages/snap-preact-components/src/providers/withTracking.tsx:84

• The isBannerTracking variable is computed during render and used in the handleClick callback, but it's not included in the callback's dependency array. While the underlying values (type, content, result, controller) are in the dependency array, it would be clearer to either move the isBannerTracking computation inside the callback or include it in the dependency array for better code maintainability and to make the dependency relationship explicit.

		const handleClick = useCallback(
			(e: MouseEvent) => {
				if (isBannerTracking) {
					(controller as SearchController | AutocompleteController)?.track.banner.click(e, content[type]![0] as MerchandisingContentBanner);
				} else {
					controller?.track.product.click(e, (result || banner)!);
				}
			},
			[controller, result, banner, type, content]

packages/snap-preact-components/src/providers/withTracking.tsx:74

• Impression tracking is performed during render (lines 67-74) rather than in a useEffect. This violates React/Preact best practices by introducing side effects during render. While the controller has deduplication logic to prevent duplicate impressions, this approach could cause issues with Concurrent Mode, makes the code harder to reason about, and could result in tracking calls on interrupted renders. Consider moving this logic into a useEffect with appropriate dependencies to ensure impressions are only tracked once per relevant state change.

		if (inViewport && !awaitingReobservationRef.current) {
			// TODO: add support for disabling tracking events via config like in ResultTracker
			if (isBannerTracking) {
				(controller as SearchController | AutocompleteController)?.track.banner.impression(content[type]![0] as MerchandisingContentBanner);
			} else if (!result?.bundleSeed) {
				controller?.track.product.impression((result || banner)!);
			}
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

packages/snap-preact-components/src/hooks/useIntersectionAdvanced.tsx:28

• The updateRef callback has an empty dependency array but it closes over the ref parameter. While ref is a mutable object and won't change identity, this pattern is unconventional. Consider adding ref to the dependency array for clarity, or add a comment explaining why the empty array is intentional.

	const updateRef = useCallback((el: HTMLElement | null) => {
		ref.current = el;
		setCounter((c) => c + 1);
	}, []);

packages/snap-preact-components/src/hooks/useIntersectionAdvanced.tsx:136

• The useEffect depends on ref which is a mutable object that doesn't change identity between renders. This means the effect won't re-run when ref.current changes unless counter changes. While this appears intentional (the counter mechanism triggers re-runs), having ref in the dependency array is misleading. Consider removing ref from the dependencies and documenting that the effect re-runs via the counter state.

	}, [ref, counter]);

packages/snap-preact-components/src/providers/withTracking.tsx:74

• Impression tracking fires during render (lines 67-74) which is a side effect in the render phase. While the tracking methods have deduplication logic to prevent duplicate impressions, React best practices recommend moving side effects into useEffect hooks. Consider wrapping this logic in a useEffect that depends on inViewport, awaitingReobservationRef.current, and the tracking entity to ensure it only fires when necessary.

		if (inViewport && !awaitingReobservationRef.current) {
			// TODO: add support for disabling tracking events via config like in ResultTracker
			if (isBannerTracking) {
				(controller as SearchController | AutocompleteController)?.track.banner.impression(content[type]![0] as MerchandisingContentBanner);
			} else if (!result?.bundleSeed) {
				controller?.track.product.impression((result || banner)!);
			}
		}

packages/snap-preact-components/src/providers/withTracking.tsx:63

• The useEffect at lines 59-63 sets awaitingReobservationRef.current = false when inViewport is false, but it also depends on resultIdentity. If resultIdentity changes while inViewport is false, this effect will run but the condition won't be met, doing nothing. The resultIdentity dependency appears unnecessary since the logic only cares about the inViewport state transitioning to false. Consider removing resultIdentity from the dependency array.

		useEffect(() => {
			if (awaitingReobservationRef.current && !inViewport) {
				awaitingReobservationRef.current = false;
			}
		}, [inViewport, resultIdentity]);

packages/snap-controller/src/Autocomplete/AutocompleteController.ts:812

• The code checks this.store.results[0]?.responseId before calling this.store.update(response) to compare with the new responseId. However, this assumes that if results exist, they have a responseId property. If the store is in an inconsistent state (e.g., results exist but lack responseId), the comparison will fail silently. Consider adding a check or comment explaining this assumption.

			const previousResponseId = this.store.results[0]?.responseId;
			if (previousResponseId && previousResponseId === responseId) {

packages/snap-preact-components/src/providers/withTracking.tsx:57

• The useEffect at lines 52-57 calls updateRef(ref.current) but doesn't include ref in its dependency array. While ref is a mutable object that doesn't change identity, React's exhaustive-deps rule expects it to be listed. Consider adding ref to the dependency array for consistency with React conventions, even though it won't cause re-runs.

		useEffect(() => {
			if (prevIdentityRef.current !== resultIdentity) {
				prevIdentityRef.current = resultIdentity;
				updateRef(ref.current);
			}
		}, [resultIdentity, updateRef]);

packages/snap-preact-components/src/providers/withTracking.tsx:85

• isBannerTracking is calculated during render (line 65) but not included in the handleClick dependency array. This means the callback captures a stale value of isBannerTracking if the tracking type changes between renders. Either add isBannerTracking to the dependency array or inline its calculation inside the callback to ensure it always reflects the current tracking type.

		const handleClick = useCallback(
			(e: MouseEvent) => {
				if (isBannerTracking) {
					(controller as SearchController | AutocompleteController)?.track.banner.click(e, content[type]![0] as MerchandisingContentBanner);
				} else {
					controller?.track.product.click(e, (result || banner)!);
				}
			},
			[controller, result, banner, type, content]
		);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.