fix: use useCallback as trackingRef to observe ref changes

[dkonieczek]

No description provided.

[Copilot]

Pull request overview

Updates intersection/impression tracking to handle element ref changes by extending useIntersectionAdvanced to return { inViewport, updateRef } and wiring a callback-ref through tracking utilities.

Changes:

• Change useIntersectionAdvanced return type from boolean to { inViewport, updateRef }.
• Propagate updateRef through createImpressionObserver and withTracking to support callback-ref behavior.
• Update infinite scroll logic in the demo to use the new inViewport field.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
packages/snap-preact-demo/src/components/Results/Results.tsx	Adopts new useIntersectionAdvanced return shape for infinite scroll trigger.
packages/snap-preact-components/src/utilities/createImpressionObserver.ts	Exposes updateRef from useIntersectionAdvanced to consumers.
packages/snap-preact-components/src/providers/withTracking.tsx	Switches trackingRef to a callback-ref and attempts to wire updateRef into tracking.
packages/snap-preact-components/src/hooks/useIntersectionAdvanced.tsx	Introduces updateRef and changes hook API to return { inViewport, updateRef }.

Comments suppressed due to low confidence (1)

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

• Calling setIntersecting(false) during render is a React/Preact anti-pattern and can trigger warnings and hard-to-reason render loops. Move this reset logic into an effect that runs when resetKey changes (and perform the cleanup there) rather than setting state synchronously in the render path.

import { useState, useEffect, useRef, MutableRef } from 'preact/hooks';

---

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

[Copilot]

useRef<HTMLElement>(null) conflicts with strictNullChecks (null is not assignable to HTMLElement) and with the hook signature MutableRef<HTMLElement | null>. This should be useRef<HTMLElement | null>(null) so the types match and assignments like ref.current = el remain valid.

Suggested change

	const ref = useRef<HTMLElement>(null);
	const ref = useRef<HTMLElement | null>(null);

[Copilot]

This callback has an empty dependency array but captures updateRef (and ref), which can become stale since updateRef is currently recreated on each useIntersectionAdvanced render. Also, the condition !ref.current prevents updating the observer when the element changes (the stated goal of the PR). Consider (1) making updateRef stable (e.g., via useCallback inside the hook) and including it in deps, and (2) updating whenever el differs from ref.current (and handling null to clear on unmount).

Suggested change

	trackingRef: useCallback((el: HTMLElement | null) => {
	if (!ref.current && el?.getAttribute(TRACKING_ATTRIBUTE) !== 'true') {
	updateRef(el);
	}
	}, []),
	trackingRef: useCallback(
	(el: HTMLElement | null) => {
	// Update observer whenever the element changes, and clear on unmount.
	if (el !== ref.current && el?.getAttribute(TRACKING_ATTRIBUTE) !== 'true') {
	updateRef(el);
	} else if (!el && ref.current) {
	updateRef(null);
	}
	},
	[ref, updateRef]
	),

[Copilot]

Including ref.current in a dependency array does not reliably rerun effects when the underlying DOM node changes, because mutating .current does not trigger a render. As a result, the click listener may remain attached to an old element when the ref changes. Prefer driving this effect off a piece of state that changes when the element changes (e.g., a node state set by a callback-ref / updateRef).

[Copilot]

updateRef is recreated every render and always increments counter, even if the same element is passed again. Consider memoizing updateRef (so consumers can safely depend on it) and only bumping counter when el !== ref.current to avoid unnecessary renders/effect re-runs.

[Copilot]

Depending on ref.current is misleading because changes to .current do not trigger re-renders; counter is already the actual signal used to force this effect. Consider removing ref.current from the dependency array to reduce confusion and avoid noisy linting; rely on counter/resetKey (or a stable node state) instead.

Suggested change

	}, [ref.current, resetKey, counter]);
	}, [resetKey, counter]);

[Copilot]

Pull request overview

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

---

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

[Copilot]

setIntersecting(false) is executed during render (inside the if (resetKey !== lastResetKeyRef.current) block). Updating state during render can cause render loops and React/Preact warnings. Move the reset logic into an effect that runs when resetKey changes (e.g., useEffect(() => { ... }, [resetKey])) and avoid returning early from the render path.

[Copilot]

This callback only calls updateRef when ref.current is falsy, so if the tracked element changes (or remounts) after the first assignment, the observer may keep observing the stale element and never update. Consider updating whenever the element identity changes (e.g., ref.current !== el) and also handling el === null to clear/unobserve appropriately.

Suggested change

	if (!ref.current && el?.getAttribute(TRACKING_ATTRIBUTE) !== 'true') {
	updateRef(el);
	}
	},
	[ref, updateRef]
	// Update the observer whenever the underlying element instance changes,
	// including when it becomes null on unmount.
	if (ref.current !== el) {
	updateRef(el);
	}
	},
	[updateRef]

[Copilot]

updateRef is recreated on every render, which makes downstream memoization (e.g., the useCallback in withTracking) ineffective and can cause extra rerenders. Wrap updateRef in useCallback (and, if needed, switch counter to useReducer/useRef-based invalidation) so consumers can keep a stable function reference.