Skip to content
Open
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
24 changes: 13 additions & 11 deletions packages/shared/src/hooks/useFeed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { usePlusSubscription } from './usePlusSubscription';
import { LogEvent } from '../lib/log';
import { useLogContext } from '../contexts/LogContext';
import type { FeedAdTemplate } from '../lib/feed';
import { getAdSlotIndex } from '../lib/feed';
import { featureFeedAdTemplate } from '../lib/featureManagement';
import { cloudinaryPostImageCoverPlaceholder } from '../lib/image';
import { AD_PLACEHOLDER_SOURCE_ID } from '../lib/constants';
Expand Down Expand Up @@ -318,21 +319,20 @@ export default function useFeed<T>(
adTemplate?.adStart ??
featureFeedAdTemplate.defaultValue.default.adStart;
const adRepeat = adTemplate?.adRepeat ?? pageSize + 1;
const adJitter = adTemplate?.adJitter ?? 0;

const adPage = getAdSlotIndex({
index,
adStart,
adRepeat,
adJitter,
seed: JSON.stringify(feedQueryKey),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to set a seed here? I think it will position the ad in the same spot for the same user. Double check it

});

const adIndex = index - adStart; // 0-based index from adStart

// if adIndex is negative, it means we are not supposed to show ads yet based on adStart
if (adIndex < 0) {
return undefined;
}
const adMatch = adIndex % adRepeat === 0; // should ad be shown at this index based on adRepeat

if (!adMatch) {
if (adPage === undefined) {
return undefined;
}

const adPage = adIndex / adRepeat; // page number for ad

if (isLoading) {
return createPlaceholderItem(adPage);
}
Expand Down Expand Up @@ -365,8 +365,10 @@ export default function useFeed<T>(
isLoading,
adTemplate?.adStart,
adTemplate?.adRepeat,
adTemplate?.adJitter,
adsUpdatedAt,
pageSize,
feedQueryKey,
],
);

Expand Down
137 changes: 137 additions & 0 deletions packages/shared/src/lib/feed.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
import { getAdSlotIndex } from './feed';

describe('getAdSlotIndex', () => {
const seed = '["feed","my-feed"]';

it('matches modulo math when adJitter is 0', () => {
const adStart = 2;
const adRepeat = 5;
const positions: number[] = [];
for (let index = 0; index < 40; index += 1) {
const n = getAdSlotIndex({ index, adStart, adRepeat, seed });
if (n !== undefined) {
positions.push(index);
}
}
expect(positions).toEqual([2, 7, 12, 17, 22, 27, 32, 37]);
});

it('returns undefined for indices before adStart (no jitter)', () => {
const adStart = 2;
const adRepeat = 5;
expect(
getAdSlotIndex({ index: 0, adStart, adRepeat, seed }),
).toBeUndefined();
expect(
getAdSlotIndex({ index: 1, adStart, adRepeat, seed }),
).toBeUndefined();
});

it('returns undefined for non-positive adRepeat', () => {
expect(
getAdSlotIndex({ index: 2, adStart: 2, adRepeat: 0, seed }),
).toBeUndefined();
});

it('never places the first ad before adStart, even with jitter', () => {
const adStart = 2;
const adRepeat = 5;
const adJitter = 2;
const seeds = Array.from({ length: 50 }, (_, i) => `["feed","user-${i}"]`);
seeds.forEach((s) => {
let firstHit: number | undefined;
for (let index = 0; index < adRepeat; index += 1) {
if (
getAdSlotIndex({ index, adStart, adRepeat, adJitter, seed: s }) !==
undefined
) {
firstHit = index;
break;
}
}
expect(firstHit).toBeDefined();
expect(firstHit).toBeGreaterThanOrEqual(adStart);
expect(firstHit).toBeLessThanOrEqual(adStart + adJitter);
});
});

it('keeps jittered positions inside the expected window per slot', () => {
const adStart = 2;
const adRepeat = 5;
const adJitter = 2;
const windows = new Map<number, number[]>();
for (let index = 0; index < 60; index += 1) {
const n = getAdSlotIndex({
index,
adStart,
adRepeat,
adJitter,
seed,
});
if (n !== undefined) {
const list = windows.get(n) ?? [];
list.push(index);
windows.set(n, list);
}
}
Array.from(windows.entries()).forEach(([n, hits]) => {
expect(hits).toHaveLength(1);
const center = adStart + n * adRepeat;
const lowerBound = n === 0 ? adStart : center - adJitter;
expect(hits[0]).toBeGreaterThanOrEqual(lowerBound);
expect(hits[0]).toBeLessThanOrEqual(center + adJitter);
});
expect(windows.size).toBeGreaterThanOrEqual(10);
});

it('is deterministic for the same seed and slot index', () => {
const args = {
adStart: 2,
adRepeat: 5,
adJitter: 2,
seed,
};
const runA: Array<number | undefined> = [];
const runB: Array<number | undefined> = [];
for (let index = 0; index < 40; index += 1) {
runA.push(getAdSlotIndex({ index, ...args }));
runB.push(getAdSlotIndex({ index, ...args }));
}
expect(runA).toEqual(runB);
});

it('produces different positions for different seeds', () => {
const args = { adStart: 2, adRepeat: 5, adJitter: 2 };
const collect = (s: string): number[] => {
const hits: number[] = [];
for (let index = 0; index < 60; index += 1) {
if (getAdSlotIndex({ index, ...args, seed: s }) !== undefined) {
hits.push(index);
}
}
return hits;
};
const seedA = '["feed","user-a"]';
const seedB = '["feed","user-b"]';
expect(collect(seedA)).not.toEqual(collect(seedB));
});

it('clamps jitter so consecutive ads never overlap or reorder', () => {
const adStart = 2;
const adRepeat = 5;
const adJitter = 100;
const hits: number[] = [];
for (let index = 0; index < 200; index += 1) {
if (
getAdSlotIndex({ index, adStart, adRepeat, adJitter, seed }) !==
undefined
) {
hits.push(index);
}
}
expect(hits.length).toBeGreaterThan(5);
for (let i = 1; i < hits.length; i += 1) {
expect(hits[i]).toBeGreaterThan(hits[i - 1]);
}
});
});
48 changes: 48 additions & 0 deletions packages/shared/src/lib/feed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,54 @@ export const getFeedName = (
export type FeedAdTemplate = {
adStart: number;
adRepeat?: number;
adJitter?: number;
};

/* eslint-disable no-bitwise -- intentional bitwise ops for FNV-1a hash */
const hashSeed = (key: string, n: number): number => {
let h = 2166136261 >>> 0;
const s = `${key}:${n}`;
for (let i = 0; i < s.length; i += 1) {
h ^= s.charCodeAt(i);
h = Math.imul(h, 16777619) >>> 0;
}
return h;
};
/* eslint-enable no-bitwise */

export const getAdSlotIndex = ({
index,
adStart,
adRepeat,
adJitter = 0,
seed,
}: {
index: number;
adStart: number;
adRepeat: number;
adJitter?: number;
seed: string;
}): number | undefined => {
if (adRepeat <= 0) {
return undefined;
}
if (index < adStart) {
return undefined;
}
const safeJitter = Math.max(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, the implementation limits the jitter based on adRepeat, which means if the repeat is every 3 posts, we can end up having post, post, ad, ad, post, .... this isn't what we want right? basically we want to give dynamic range

0,
Math.min(adJitter, Math.floor((adRepeat - 1) / 2)),
);
const n = Math.max(0, Math.round((index - adStart) / adRepeat));
// For n === 0, only allow non-negative jitter so the first ad never lands
// before adStart. For n > 0, jitter is symmetric around the cadence.
const isFirst = n === 0;
const range = isFirst ? safeJitter + 1 : safeJitter * 2 + 1;
const baseOffset = isFirst ? 0 : -safeJitter;
const offset =
safeJitter === 0 ? 0 : (hashSeed(seed, n) % range) + baseOffset;
const pos = adStart + n * adRepeat + offset;
return pos === index ? n : undefined;
};

export function usePostLogEvent() {
Expand Down
Loading