diff --git a/.changeset/range-replacement-fixes.md b/.changeset/range-replacement-fixes.md new file mode 100644 index 00000000..444e6cae --- /dev/null +++ b/.changeset/range-replacement-fixes.md @@ -0,0 +1,10 @@ +--- +"@monetizationos/proxy": patch +--- + +Range replacement fixes: + +- Uses less CPU on pages with many void elements (`img`, `br`, `meta`, ...). Output is unchanged. +- A range container with no end tag (`img`, or a self-closing SVG element such as ``) is left as it is instead of failing the whole page transform. +- A range starts at the first start marker, so a start marker nested inside it no longer inserts the replacement a second time. +- A range applies to the outermost matching container. A container nested inside another one is part of the outer container's cut. diff --git a/src/stages/buildReplacementHandlers.ts b/src/stages/buildReplacementHandlers.ts index 2acd9a26..9983fcb7 100644 --- a/src/stages/buildReplacementHandlers.ts +++ b/src/stages/buildReplacementHandlers.ts @@ -3,9 +3,11 @@ import type { HtmlStageContext } from '../context' import type { SubSurfaceBehaviorApi } from '../types' import { renderElement } from './elements' import type { ComponentRange } from './findMarkerPositions' +import { isVoidElement } from './voidElements' type ReplacementState = { - operation: 'SCANNING' | 'REMOVING' | 'REPLACED' + // `IN_START`: a start marker nested in the first one doesn't start the range again + operation: 'SCANNING' | 'IN_START' | 'REMOVING' | 'REPLACED' } export type ReplacementHandler = { selector: string; handler: ElementHandlers } @@ -69,7 +71,8 @@ export const buildReplacementHandlers = ( const [content, options] = renderElement(ctx, after) element.after(content, options) } - if (element.onEndTag) { + state.operation = 'IN_START' + if (element.onEndTag && !isVoidElement(element)) { try { element.onEndTag(() => { state.operation = 'REMOVING' @@ -99,27 +102,47 @@ export const buildReplacementHandlers = ( } } + // Open containers; a container inside another one is part of the outer one's range + let depth = 0 + const parentHandler: ElementHandlers = { element(element: RewriterElement) { + if (depth > 0) { + return + } rangeIndex++ - if (element.removed) { + // No content to cut + if (isVoidElement(element)) { return } + // A removed container still counts towards `depth`, so the ranges line up with the marker pass + const state: ReplacementState | null = element.removed ? null : { operation: modification.fromMarker ? 'SCANNING' : 'REMOVING' } + if (state) { + states.push(state) + } - const state: ReplacementState = { operation: modification.fromMarker ? 'SCANNING' : 'REMOVING' } - states.push(state) - - if (state.operation === 'REMOVING') { - for (const prepend of (modification.replaceWith ?? []).slice().reverse()) { - const [content, options] = renderElement(ctx, prepend) - element.prepend(content, options) + try { + if (state?.operation === 'REMOVING') { + for (const prepend of (modification.replaceWith ?? []).slice().reverse()) { + const [content, options] = renderElement(ctx, prepend) + element.prepend(content, options) + } } - } - if (element.onEndTag) { - element.onEndTag(() => { + if (element.onEndTag) { + element.onEndTag(() => { + depth-- + if (state) { + states.pop() + } + }) + depth++ + } + } catch { + // `onEndTag` throws on a self-closing foreign container (``), which has no end tag or content to cut + if (state) { states.pop() - }) + } } }, diff --git a/src/stages/findMarkerPositions.ts b/src/stages/findMarkerPositions.ts index f64e933b..a06347fb 100644 --- a/src/stages/findMarkerPositions.ts +++ b/src/stages/findMarkerPositions.ts @@ -1,6 +1,7 @@ import type { ElementHandlers, HtmlRewriterAdapter, RewriterElement } from '../adapters/HtmlRewriterAdapter' import type { HtmlStageContext } from '../context' import type { SurfaceDecisionResponse } from '../types' +import { isVoidElement } from './voidElements' export type SurfaceMarkers = { componentsWithInvalidSelectors: string[] @@ -52,15 +53,30 @@ export const findMarkerPositions = async ( const { toMarker, fromMarker } = modification let elementCounter = 0 let currentRange: ComponentRange | null = null + // Open containers; a container inside another one is part of the outer one's range + let depth = 0 const entries: (Handler | null)[] = [ { selector: `${componentBehavior.metadata.cssSelector}`, componentKey, handler: { - element: () => { + element: (element: RewriterElement) => { + if (depth > 0) { + return + } currentRange = { markers: [] } markers[componentKey]?.push(currentRange) + if (element.onEndTag && !isVoidElement(element)) { + try { + element.onEndTag(() => { + depth-- + }) + depth++ + } catch { + // No end tag, so nothing can be nested in it + } + } }, }, }, @@ -81,7 +97,7 @@ export const findMarkerPositions = async ( nextEndMarker: null, } currentRange.markers[elementCounter] = marker - if (element.onEndTag) { + if (element.onEndTag && !isVoidElement(element)) { try { element.onEndTag(() => { marker.endTag = elementCounter diff --git a/src/stages/voidElements.ts b/src/stages/voidElements.ts new file mode 100644 index 00000000..a77c09e0 --- /dev/null +++ b/src/stages/voidElements.ts @@ -0,0 +1,24 @@ +import type { RewriterElement } from '../adapters/HtmlRewriterAdapter' + +const VOID_ELEMENTS = new Set([ + 'area', + 'base', + 'br', + 'col', + 'embed', + 'hr', + 'img', + 'input', + 'link', + 'meta', + 'param', + 'source', + 'track', + 'wbr', +]) + +/** + * HTML void elements have no end tag, and rewriter runtimes throw when `onEndTag` is called on them. Self-closing + * foreign elements (`` in SVG) throw too but cannot be recognised by name, so callers keep a try/catch. + */ +export const isVoidElement = (element: RewriterElement): boolean => VOID_ELEMENTS.has(element.tagName) diff --git a/test/stages/surfaceComponents.spec.ts b/test/stages/surfaceComponents.spec.ts index 5bf53baa..b7b79220 100644 --- a/test/stages/surfaceComponents.spec.ts +++ b/test/stages/surfaceComponents.spec.ts @@ -332,6 +332,78 @@ describe('handleSurfaceComponents', () => { expect(events).toContainEqual(expect.objectContaining({ level: 'warn', code: 'replacement-markers-identical' })) }) + describe('replaceRange with nested matches', () => { + const replaceRange = async (body: string, range: { fromMarker?: string }, cssSelector = '.c') => { + const decision = decisions({ + range: { + metadata: { cssSelector }, + content: { replaceRange: { ...range, replaceWith: [{ type: 'html', content: 'NEW' }] } }, + }, + }) + return (await handleSurfaceComponents(baseCtx, html(body), decision, new LolHtmlRewriter())).text() + } + + it('starts the range at the first start marker when it holds another', async () => { + const body = await replaceRange('

a

b

c

', { + fromMarker: '.s', + }) + expect(body).toBe('

a

b

NEW
') + }) + + it('starts the range at the first start marker when both are nested in the container', async () => { + const body = await replaceRange( + '

a

b

c

', + { + fromMarker: '.s', + }, + ) + expect(body).toBe('

a

NEW
') + }) + + it('applies the range to the outermost of nested containers', async () => { + const body = await replaceRange( + '

a

b

c

d

e

f

', + { fromMarker: 'p.s' }, + ) + expect(body).toBe( + '

a

NEW

e

NEW
', + ) + }) + + it('replaces the whole outermost container when nested containers have no markers', async () => { + const body = await replaceRange('

a

b

c

', {}) + expect(body).toBe('
NEW
') + }) + + it('still cuts a later container after one removed by another component', async () => { + const decision = decisions({ + gone: { metadata: { cssSelector: '#gone' }, content: { remove: true } }, + range: { + metadata: { cssSelector: '.c' }, + content: { replaceRange: { fromMarker: 'p.s', replaceWith: [{ type: 'html', content: 'NEW' }] } }, + }, + }) + const page = + '

a

b

e

f

' + const result = await handleSurfaceComponents(baseCtx, html(page), decision, new LolHtmlRewriter()) + expect(await result.text()).toBe('

e

NEW
') + }) + + it('applies the range to a container matched by a selector list', async () => { + const body = await replaceRange('

a

b

', { fromMarker: 'p.s' }, '.x, .c') + expect(body).toBe('

a

NEW
') + }) + + it('applies the range to the outermost of nested containers matched by a descendant selector', async () => { + const body = await replaceRange( + '

a

b

c

d

', + { fromMarker: 'p.s' }, + 'main .c', + ) + expect(body).toBe('

a

NEW
') + }) + }) + it('falls back gracefully when findMarkerPositions rejects a marker selector — other components still apply', async () => { const { events, logger } = createRecordingLogger() const response = html('
leave
orig
') diff --git a/test/stages/voidElements.spec.ts b/test/stages/voidElements.spec.ts new file mode 100644 index 00000000..d046ac3e --- /dev/null +++ b/test/stages/voidElements.spec.ts @@ -0,0 +1,175 @@ +import { describe, expect, it } from 'vitest' +import type { ElementHandlers, HtmlRewriterAdapter, HtmlRewriterSession, RewriterElement } from '../../src/adapters/HtmlRewriterAdapter' +import { normalizeMOSConfig } from '../../src/config' +import type { PipelineContext } from '../../src/context' +import handleSurfaceComponents from '../../src/stages/surfaceComponents' +import { isVoidElement } from '../../src/stages/voidElements' +import type { SurfaceDecisionResponse } from '../../src/types' +import { LolHtmlRewriter } from '../fakes/LolHtmlRewriter' + +const ctx: PipelineContext = { + config: normalizeMOSConfig({ + originUrl: 'https://origin.example.com', + surfaceSlug: 'web', + mosHost: 'https://api.monetizationos.com', + mosSecretKey: 'sk_env_test_abc', + anonymousSessionCookieName: 'anon-session', + authenticatedUserJwtCookieName: '__session', + }), + logger: { log() {} }, +} + +/** Wraps the real rewriter and records the tag of every element `onEndTag` is called on. */ +class EndTagSpyRewriter implements HtmlRewriterAdapter { + readonly inner = new LolHtmlRewriter() + readonly capabilities = this.inner.capabilities + readonly endTagCalls: string[] = [] + + // Methods are bound to the element, which a real rewriter element needs + private spyOn(element: RewriterElement): RewriterElement { + const onEndTag: RewriterElement['onEndTag'] = (callback) => { + this.endTagCalls.push(element.tagName.toLowerCase()) + element.onEndTag?.(callback) + } + return new Proxy(element, { + get: (target, prop) => { + if (prop === 'onEndTag') { + return onEndTag + } + const value = Reflect.get(target, prop) + return typeof value === 'function' ? value.bind(target) : value + }, + }) + } + + create(): HtmlRewriterSession { + const session = this.inner.create() + const spy: HtmlRewriterSession = { + on: (selector: string, handlers: ElementHandlers) => { + session.on(selector, { + ...handlers, + element: handlers.element ? (element) => handlers.element?.(this.spyOn(element)) : undefined, + }) + return spy + }, + transform: (response) => session.transform(response), + } + return spy + } +} + +const decisions = (replaceRange: { fromMarker?: string; toMarker?: string }, cssSelector = 'div.hero'): SurfaceDecisionResponse => ({ + status: 'success', + identity: { identifier: 'anon', isAuthenticated: false, authType: 'anonymous', jwtClaims: {} }, + features: {}, + customer: { hasProducts: false }, + surfaceBehavior: {}, + componentsSkipped: false, + componentBehaviors: { + hero: { + metadata: { cssSelector }, + content: { replaceRange: { ...replaceRange, replaceWith: [{ type: 'html', content: 'NEW' }] } }, + }, + }, +}) + +const run = async (body: string, replaceRange: { fromMarker?: string; toMarker?: string }, cssSelector?: string) => { + const rewriter = new EndTagSpyRewriter() + const response = new Response(body, { headers: { 'Content-Type': 'text/html; charset=utf-8' } }) + const result = await handleSurfaceComponents(ctx, response, decisions(replaceRange, cssSelector), rewriter) + return { body: await result.text(), endTagCalls: rewriter.endTagCalls } +} + +describe('range replacement with void elements', () => { + const VOID_TAGS = ['br', 'img', 'hr', 'input', 'meta', 'link', 'wbr'] + + it('does not call onEndTag on void elements and replaces the range as before', async () => { + const { body, endTagCalls } = await run( + '
S

kill

E
', + { fromMarker: 'span.start', toMarker: 'span.end' }, + ) + expect(body).toBe('
SNEWE
') + for (const tag of VOID_TAGS) { + expect(endTagCalls).not.toContain(tag) + } + expect(endTagCalls).toContain('p') + }) + + it('cuts past void elements written with an end tag, which the cut leaves behind', async () => { + const { body, endTagCalls } = await run( + '
S

kill

E
', + { fromMarker: 'span.start', toMarker: 'span.end' }, + ) + // The rewriter never hands a stray end tag to a handler, so cutting element by element can't remove it + expect(body).toBe('
SNEW
E
') + expect(endTagCalls).not.toContain('img') + expect(endTagCalls).not.toContain('br') + }) + + it('handles a void element as the fromMarker without calling onEndTag on it', async () => { + const { body, endTagCalls } = await run( + '

keep

kill

E
', + { fromMarker: 'hr.start', toMarker: 'span.end' }, + ) + expect(body).toBe('

keep

NEWE
') + expect(endTagCalls).not.toContain('hr') + }) +}) + +describe('isVoidElement', () => { + const element = (tagName: string) => ({ tagName }) as RewriterElement + + it.each([ + 'area', + 'base', + 'br', + 'col', + 'embed', + 'hr', + 'img', + 'input', + 'link', + 'meta', + 'param', + 'source', + 'track', + 'wbr', + ])('treats <%s> as void', (tag) => { + expect(isVoidElement(element(tag))).toBe(true) + }) + + it('does not treat elements with an end tag as void', () => { + expect(isVoidElement(element('p'))).toBe(false) + }) +}) + +describe('range replacement when the container has no end tag', () => { + it.each([ + ['without markers', {}], + ['with a fromMarker', { fromMarker: 'span' }], + ])('leaves a void container alone %s', async (_, replaceRange) => { + const html = '

a

b

' + const { body } = await run(html, replaceRange, 'img.hero') + expect(body).toBe(html) + }) + + it('leaves a self-closing foreign container alone', async () => { + const html = '

b

' + const { body } = await run(html, {}, 'path.hero') + expect(body).toBe(html) + }) + + it('pops the state of a void container nested in a matching container', async () => { + const { body } = await run( + '
s

x

', + { fromMarker: '.s' }, + '.hero', + ) + expect(body).toBe('
sNEW
') + }) + + it('still replaces a later container once an earlier match had no end tag', async () => { + const { body } = await run('

x

', {}, '.hero') + expect(body).toBe('
NEW
') + }) +})