MD-3007 Fix replaceRange edge cases

.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 `<path/>`) 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.

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 (`<path/>`), which has no end tag or content to cut
+                if (state) {
                     states.pop()
-                })
+                }
             }
         },
 

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

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 (`<path/>` 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)

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: '<i>NEW</i>' }] } },
+                },
+            })
+            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('<div class="c"><div class="s"><p class="s">a</p><p>b</p></div><p>c</p></div>', {
+                fromMarker: '.s',
+            })
+            expect(body).toBe('<div class="c"><div class="s"><p class="s">a</p><p>b</p></div><i>NEW</i></div>')
+        })
+
+        it('starts the range at the first start marker when both are nested in the container', async () => {
+            const body = await replaceRange(
+                '<div class="c"><section><div class="s"><p class="s">a</p></div><p>b</p></section><p>c</p></div>',
+                {
+                    fromMarker: '.s',
+                },
+            )
+            expect(body).toBe('<div class="c"><section><div class="s"><p class="s">a</p></div><i>NEW</i></section></div>')
+        })
+
+        it('applies the range to the outermost of nested containers', async () => {
+            const body = await replaceRange(
+                '<div class="c"><div class="c"><p class="s">a</p><p>b</p></div><p class="s">c</p><p>d</p></div><div class="c"><p class="s">e</p><p>f</p></div>',
+                { fromMarker: 'p.s' },
+            )
+            expect(body).toBe(
+                '<div class="c"><div class="c"><p class="s">a</p><i>NEW</i></div></div><div class="c"><p class="s">e</p><i>NEW</i></div>',
+            )
+        })
+
+        it('replaces the whole outermost container when nested containers have no markers', async () => {
+            const body = await replaceRange('<div class="c"><p>a</p><div class="c"><p>b</p></div><p>c</p></div>', {})
+            expect(body).toBe('<div class="c"><i>NEW</i></div>')
+        })
+
+        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: '<i>NEW</i>' }] } },
+                },
+            })
+            const page =
+                '<div class="c" id="gone"><div class="c"><p class="s">a</p><p>b</p></div></div><div class="c"><p class="s">e</p><p>f</p></div>'
+            const result = await handleSurfaceComponents(baseCtx, html(page), decision, new LolHtmlRewriter())
+            expect(await result.text()).toBe('<div class="c"><p class="s">e</p><i>NEW</i></div>')
+        })
+
+        it('applies the range to a container matched by a selector list', async () => {
+            const body = await replaceRange('<div class="c"><p class="s">a</p><p>b</p></div>', { fromMarker: 'p.s' }, '.x, .c')
+            expect(body).toBe('<div class="c"><p class="s">a</p><i>NEW</i></div>')
+        })
+
+        it('applies the range to the outermost of nested containers matched by a descendant selector', async () => {
+            const body = await replaceRange(
+                '<main><div class="c"><div class="c"><p class="s">a</p><p>b</p></div><p class="s">c</p><p>d</p></div></main>',
+                { fromMarker: 'p.s' },
+                'main .c',
+            )
+            expect(body).toBe('<main><div class="c"><div class="c"><p class="s">a</p><i>NEW</i></div></div></main>')
+        })
+    })
+
     it('falls back gracefully when findMarkerPositions rejects a marker selector — other components still apply', async () => {
         const { events, logger } = createRecordingLogger()
         const response = html('<html><body><div class="bad">leave</div><div class="ok">orig</div></body></html>')