From 4a551482ec89de977e0f7f6919c982d02bee3e06 Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 6 Aug 2026 20:08:59 -0400 Subject: [PATCH] feat(browser): scope waitFor rules by page type instead of a duplicated path regex; v1.17.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `waitFor[].pathPattern` describes which pages have a widget as a regex over the URL path — a SECOND description of a routing fact the plugin's route list already owns, maintained in a different repository. The two drift silently: add a route to a template on the plugin side and it renders without the rule, with nothing reporting the gap. The plugin (prerender-v0.34.0) now declares named page types and sends the matched name on each queue job. This consumes it: RenderJob carries `pageType` from the job payload (absent on older plugins). WaitForRule gains `pageTypes?: string[]`, ANDed with `devices`. A job with no declared type matches no such rule — skipping costs that rule's content, while applying it blindly would restore the poll-to- timeout on every page lacking the widget that scoping exists to prevent. renderOnce gains a `pageType` option, so the harness reproduces a scoped rule instead of silently rendering what the fleet does not. renderAudit its `pageType` was a REPORT LABEL that never reached the render. Left alone, a pageTypes-scoped rule would be skipped on every audit render, so state B would settle differently from the fleet it reproduces and the audit would report content as missing that production captures. It now scopes the render too. suggest emits `pageTypes: [name]` over `pathPattern` when the audit was given a type — a suggested regex is the same second copy of the route list, drifting from the moment a route is added. `pathPattern` still works and is not deprecated; migrate rule by rule. --- package-lock.json | 2 +- packages/browser/README.md | 35 ++++++++++++++++++--- packages/browser/package.json | 2 +- packages/browser/src/RenderJob.ts | 13 ++++++++ packages/browser/src/audit/renderAudit.ts | 8 ++++- packages/browser/src/audit/suggest.ts | 19 ++++++++---- packages/browser/src/config.ts | 28 +++++++++++++++++ packages/browser/src/renderOnce.ts | 9 ++++++ packages/browser/src/renderer.ts | 13 +++++--- packages/browser/test/config.test.ts | 20 ++++++++++++ packages/browser/test/renderOnce.test.ts | 38 +++++++++++++++++++++++ 11 files changed, 170 insertions(+), 17 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9473882..901cf77 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8967,7 +8967,7 @@ }, "packages/browser": { "name": "@harperfast/prerender-browser", - "version": "1.16.0", + "version": "1.17.0", "license": "Apache-2.0", "dependencies": { "mqtt": "^5.10.4", diff --git a/packages/browser/README.md b/packages/browser/README.md index 5d86d86..e614003 100644 --- a/packages/browser/README.md +++ b/packages/browser/README.md @@ -98,7 +98,7 @@ include what you change: "scroll": { "enabled": true, "stepMs": 200, "topSettleMs": 300 }, // scroll to bottom for lazy content; topSettleMs lets scroll-reactive headers re-reveal at the top before serializing // optional: AFTER the normal scroll-settle (which still runs and triggers all other lazy content), // scroll a selector into view and wait for lazy content (e.g. reviews below the fold on a short - // viewport) before the snapshot. Absent → no-op. Scope each rule with `devices`/`pathPattern` so it + // viewport) before the snapshot. Absent → no-op. Scope each rule with `devices`/`pageTypes` so it // only runs where the widget is — otherwise it polls to `timeoutMs` on pages/devices that lack it. "waitFor": [ { @@ -107,7 +107,7 @@ include what you change: "minCount": 1, "timeoutMs": 15000, "devices": ["mobile", "tablet"], // desktop's tall viewport already has it in view - "pathPattern": "^/product/", // only product pages have this widget + "pageTypes": ["pdp"], // only product pages have this widget }, ], "postProcess": { @@ -123,6 +123,34 @@ include what you change: Invalid config (missing viewport, `defaultDevice` not in `devices`, non-positive budgets) throws at `startWorker()`. +### Scoping a `waitFor` rule: `pageTypes` vs `pathPattern` + +An unscoped rule polls to its `timeoutMs` on every page and device that lacks the widget, so every +rule should say where it applies. There are two ways, and they AND together with `devices`: + +| scope | matches when | use when | +| ------------- | ------------------------------------------- | ------------------------------------------ | +| `pageTypes` | the job's declared page type is in the list | the plugin declares page types (preferred) | +| `pathPattern` | the URL path matches this regex | no page types declared, or a one-off URL | + +**Prefer `pageTypes`.** The name comes from the plugin's own route list +(`ingress.routes[].pageType`), so the rule is expressed in the same vocabulary that decides what +gets rendered at all — one list to keep correct instead of two. A `pathPattern` is a second +description of a routing fact the plugin already owns, and the two drift silently: add a route to +that template on the plugin side and it renders without the rule, with nothing reporting it. Page +types also express cheaply what a regex needs an alternation for — one template reached by several +unrelated URL shapes is one name. + +A job carrying **no** page type never matches a `pageTypes` rule. That happens with a plugin older +than `prerender-v0.34.0`, or a route that names no template. Skipping is the safe direction: it +costs the content this one rule would have waited for, rather than reinstating the poll-to-timeout +on every page that lacks the widget. `pathPattern` still works and is not deprecated — migrate rule +by rule. + +`renderOnce`/`renderMatrix` take a `pageType` option, and `renderAudit`'s existing `pageType` +now reaches the render as well as the report. Without it, a `pageTypes`-scoped rule is skipped and +the harness renders a page the fleet settles differently. + ## Custom renderer A renderer receives the Puppeteer `page` and the `RenderJob` and returns the serialized HTML (or @@ -192,8 +220,7 @@ const cell = await renderAudit({ bypass: { header: 'x-harper-renderer-bypass', token: process.env.TOKEN }, hostResolverRules: { 'example.com': '203.0.113.10' }, // reach a staging edge IP in this env buckets: { reviews: '[class*=review-]' }, // page-type element counts, shadow-aware - pageType: 'pdp', - pathPattern: '^/product/', + pageType: 'pdp', // groups the report AND scopes the render + any suggested waitFor rule }); console.log(cell.diff1.missing); // SEO content in the full render but absent from the served bytes diff --git a/packages/browser/package.json b/packages/browser/package.json index 04025af..e078f4c 100644 --- a/packages/browser/package.json +++ b/packages/browser/package.json @@ -1,6 +1,6 @@ { "name": "@harperfast/prerender-browser", - "version": "1.16.0", + "version": "1.17.0", "type": "module", "description": "Headless-browser render library for Harper Prerender: claims render jobs from the @harperfast/prerender queue, renders pages in headless Chrome (Puppeteer), and posts the HTML back. Embedded by a render service and configured entirely via startWorker() options.", "keywords": [ diff --git a/packages/browser/src/RenderJob.ts b/packages/browser/src/RenderJob.ts index c1aa93f..a4c7af3 100644 --- a/packages/browser/src/RenderJob.ts +++ b/packages/browser/src/RenderJob.ts @@ -32,6 +32,16 @@ export type JobConfig = { renderBudget?: number; callbackOrigin: string; isFromSitemap: boolean; + /** + * The template this URL belongs to — the plugin's declared `pageType` for the route that + * matched (`home`, `category`, `pdp`), or absent when the route names none. + * + * Lets render rules be scoped by template name instead of by a `pathPattern` regex that + * restates URL shapes the plugin's route list already owns. Absent on jobs from a plugin + * older than prerender-v0.34.0, so anything reading it must treat absence as "unknown + * template" and fall back rather than skip. + */ + pageType?: string | null; }; /** @@ -110,6 +120,8 @@ export default class RenderJob { isIndexable: boolean | undefined; redirectedTo: string | undefined; isFromSitemap: boolean; + /** Declared template name for this URL, or null/undefined when the plugin sent none. */ + pageType: string | null | undefined; /** * Why this render produced no cacheable content — one slug across every no-content class, * so the plugin logs/tracks a single field: 'noindex' (robots meta/header), @@ -136,6 +148,7 @@ export default class RenderJob { this.renderBudget = config.renderBudget; this.callbackOrigin = config.callbackOrigin; this.isFromSitemap = config.isFromSitemap; + this.pageType = config.pageType; } sanitizeHeaders(headers: Record) { diff --git a/packages/browser/src/audit/renderAudit.ts b/packages/browser/src/audit/renderAudit.ts index 7a8050c..9fc06fb 100644 --- a/packages/browser/src/audit/renderAudit.ts +++ b/packages/browser/src/audit/renderAudit.ts @@ -117,7 +117,13 @@ export async function renderAudit(o: RenderAuditOptions): Promise { const fullConfig = buildFullConfig(base); const blockUrlPatterns = (base && base.block && base.block.urlPatterns) || []; const resolvedHosts = Object.keys(hostResolverRules || {}); - const common = { device, bypass, hostResolverRules }; + // `pageType` reaches the RENDER, not just the report grouping. It was a label only, which was + // harmless while nothing in a render consulted it — but a `waitFor` rule scoped with + // `pageTypes` would then be skipped on every audit render, so state B would settle differently + // from the fleet it is supposed to reproduce and the audit would report content as missing that + // production actually captures. Empty string (the default) is passed as undefined so an + // unlabelled audit is "no page type", not a type named ''. + const common = { device, bypass, hostResolverRules, pageType: pageType || undefined }; // State A probe: hydrate, then extract BOTH modes from the live post-render page: // • structural → the Diff-1 fingerprint (bots parse the DOM; visibility is irrelevant to "is the diff --git a/packages/browser/src/audit/suggest.ts b/packages/browser/src/audit/suggest.ts index 3f539d1..bf871ec 100644 --- a/packages/browser/src/audit/suggest.ts +++ b/packages/browser/src/audit/suggest.ts @@ -85,7 +85,7 @@ function bucketNameOf( return ''; } -/** Build one waitFor rule from a lazy-widget finding, scoped to the given devices/pathPattern. +/** Build one waitFor rule from a lazy-widget finding, scoped to the given devices/page type. * Mapping (e.g. `{selector:'#reviews', waitForSelector:'[class*=review-]'}`): * selector = the specific element/container to scroll into view (finding.selectorPath), * waitForSelector = the lazy-content class to count (the bucket selector), emitted only when it is @@ -94,7 +94,7 @@ function bucketNameOf( * human-fillable rather than silently broken. */ function deriveWaitForRule( finding: Finding, - { devices, pathPattern }: { devices: string[]; pathPattern?: string } + { devices, pageType, pathPattern }: { devices: string[]; pageType?: string; pathPattern?: string } ): WaitForRule { const content = bucketSelectorOf(finding); // lazy content class → waitForSelector const anchor = trimStr(finding && finding.selectorPath); // specific element/container → selector @@ -108,8 +108,15 @@ function deriveWaitForRule( rule.minCount = 1; rule.timeoutMs = 15000; if (devices.length) rule.devices = devices; // omit → all devices (config default); avoids [undefined] + // Scope so the rule never adds latency where the widget doesn't exist. Prefer the page-type + // NAME when the audit was given one: it is the plugin's own vocabulary, so the emitted patch + // stays correct as routes are added to that template — a suggested `pathPattern` is a second + // copy of the route list that starts drifting the moment one is. Fall back to the pattern when + // the audit was run without a type. + const pt = trimStr(pageType); const pp = trimStr(pathPattern); - if (pp) rule.pathPattern = pp; // scope to the page type's routes so it never adds latency elsewhere + if (pt) rule.pageTypes = [pt]; + else if (pp) rule.pathPattern = pp; return rule; } @@ -119,8 +126,8 @@ function deriveWaitForRule( * @param {{missing?:object[], flakyB?:object[], stale?:object[], bucketDrops?:object[]}} diff1 * @param {{findings?:object[]}} diff2 * @param {object} [options] - * @param {string} [options.pageType] page-type label (context for the viewport note) - * @param {string} [options.pathPattern] regex source scoping waitFor rules to this page type's routes + * @param {string} [options.pageType] page-type name — scopes emitted waitFor rules, and context for the viewport note + * @param {string} [options.pathPattern] regex fallback for scoping waitFor rules when no pageType is given * @param {string} [options.device] the device this cell audited (default waitFor scope) * @param {string[]} [options.missingDevices] devices that actually lack the content → waitFor scope * @returns {object} minimal patch: some subset of { postProcess:{removeSelectors?,resolveLazyImages?}, @@ -170,7 +177,7 @@ export function suggestFixes( const seenRuleKeys = new Set(); // dedupe rules that collapse to the same (selector, waitForSelector) let hasTodoRule = false; for (const f of lazyCandidates) { - const rule = deriveWaitForRule(f, { devices, pathPattern }); + const rule = deriveWaitForRule(f, { devices, pageType, pathPattern }); const key = rule.selector + '\n' + (rule.waitForSelector || ''); if (seenRuleKeys.has(key)) continue; seenRuleKeys.add(key); diff --git a/packages/browser/src/config.ts b/packages/browser/src/config.ts index 3d5447b..6645299 100644 --- a/packages/browser/src/config.ts +++ b/packages/browser/src/config.ts @@ -194,8 +194,30 @@ export type WaitForRule = { * Only apply this rule when the render URL's PATH matches this JavaScript regular expression * (e.g. `'^/product/'` for PDPs). Omit → all paths. Scope a rule to the routes that have the * widget so it never polls to the timeout on pages that don't (a page-type latency guard). + * + * Prefer {@link WaitForRule.pageTypes} where the plugin declares page types: this pattern is a + * SECOND description of a routing fact the plugin's route list already owns, and the two drift + * silently — a route added there keeps rendering without the rule, and nothing reports it. */ pathPattern?: string; + /** + * Only apply this rule when the job's declared page type is one of these names (e.g. + * `['pdp']`). Omit → all page types. + * + * The template name comes from the plugin's own route list (`ingress.routes[].pageType`), so + * this expresses "the pages that have this widget" in the same vocabulary that decides what + * gets rendered at all — one list to keep correct instead of two. It also covers what a path + * regex cannot say cheaply: one template reached by several unrelated URL shapes is one name + * here, rather than an alternation that has to be kept in step by hand. + * + * A job carrying NO page type (a plugin older than prerender-v0.34.0, or a route that names + * no template) does not match any `pageTypes` rule. That is the safe direction: the rule is + * skipped, which costs the content it would have waited for, rather than applied blindly, + * which costs a poll to the timeout on every page that lacks the widget. + * + * Combines with the other scopes by AND — a rule with `devices` and `pageTypes` needs both. + */ + pageTypes?: string[]; }; export type PrerenderConfig = { @@ -343,6 +365,12 @@ const validate = (config: PrerenderConfig): PrerenderConfig => { ) { throw new Error(`prerender config: waitFor[${i}].devices must be an array of non-empty device names`); } + if ( + rule.pageTypes !== undefined && + (!Array.isArray(rule.pageTypes) || rule.pageTypes.some((t) => typeof t !== 'string' || t.trim() === '')) + ) { + throw new Error(`prerender config: waitFor[${i}].pageTypes must be an array of non-empty type names`); + } if (rule.pathPattern !== undefined) { if (typeof rule.pathPattern !== 'string' || rule.pathPattern.trim() === '') { throw new Error(`prerender config: waitFor[${i}].pathPattern must be a non-empty string`); diff --git a/packages/browser/src/renderOnce.ts b/packages/browser/src/renderOnce.ts index 1e056db..40e7ae2 100644 --- a/packages/browser/src/renderOnce.ts +++ b/packages/browser/src/renderOnce.ts @@ -54,6 +54,13 @@ export interface RenderOnceOptions extends Omit { url: string; /** Device profile key (into config.devices). Default: config.defaultDevice. */ device?: string; + /** + * Page type (template) name to render as, e.g. `'pdp'` — what the plugin would send on a real + * queue job for this URL. Required to reproduce a `waitFor` rule scoped with `pageTypes`: + * without it the job carries no template and every such rule is skipped, so the harness would + * silently render a page the fleet settles differently. + */ + pageType?: string; /** Extra per-navigation request headers (merged onto the request like a job's headers). */ headers?: Record; acceptLanguage?: string; @@ -140,6 +147,7 @@ export async function renderOnce(options: RenderOnceOptions): Promise { /* leave '' */ } for (const rule of config.waitFor ?? []) { - // Per-rule scoping: skip rules that don't target this device or path, so a rule never - // polls to its timeout on renders it isn't meant for (e.g. a PDP-reviews rule on a - // category page, or on desktop where the content is already in view). Validated at config - // load, so a bad pathPattern regex can't reach here. + // Per-rule scoping: skip rules that don't target this device, page type, or path, so a + // rule never polls to its timeout on renders it isn't meant for (e.g. a PDP-reviews rule + // on a category page, or on desktop where the content is already in view). Scopes AND + // together. Validated at config load, so a bad pathPattern regex can't reach here. if (rule.devices && !rule.devices.includes(deviceType)) continue; + // A job with no declared page type never matches a pageTypes rule — the plugin may + // predate the field, or the route may name no template. Skipping costs the content this + // rule would have waited for; applying it blindly would cost a poll to the timeout on + // every page that lacks the widget, which is the failure this scoping exists to prevent. + if (rule.pageTypes && !(job.pageType && rule.pageTypes.includes(job.pageType))) continue; if (rule.pathPattern && !new RegExp(rule.pathPattern).test(path)) continue; const contentSelector = rule.waitForSelector ?? rule.selector; diff --git a/packages/browser/test/config.test.ts b/packages/browser/test/config.test.ts index 2897d27..e60e37d 100644 --- a/packages/browser/test/config.test.ts +++ b/packages/browser/test/config.test.ts @@ -137,6 +137,26 @@ test('waitFor: validation of devices / pathPattern scoping', () => { ); }); +test('waitFor: validation of pageTypes scoping', () => { + assert.throws( + () => mergeConfig({ waitFor: [{ selector: '#r', pageTypes: 'pdp' as unknown as string[] }] }), + /waitFor\[0\]\.pageTypes must be an array/ + ); + assert.throws( + () => mergeConfig({ waitFor: [{ selector: '#r', pageTypes: ['pdp', ''] }] }), + /waitFor\[0\]\.pageTypes must be an array/ + ); + // One template reached by several URL shapes is several names here, not a regex alternation + // that has to be kept in step with the plugin's route list by hand. + assert.doesNotThrow(() => + mergeConfig({ waitFor: [{ selector: '#r', devices: ['mobile'], pageTypes: ['pdp', 'category'] }] }) + ); + // pageTypes and pathPattern coexist — the migration path is rule-by-rule, not a flag day. + assert.doesNotThrow(() => + mergeConfig({ waitFor: [{ selector: '#r', pageTypes: ['pdp'], pathPattern: '^/product/' }] }) + ); +}); + test('validation: a device must have a numeric viewport', () => { const file = writeConfig({ devices: { desktop: { viewport: { width: 'wide' } } } }); assert.throws(() => loadConfig(file), /requires a viewport with numeric width and height/); diff --git a/packages/browser/test/renderOnce.test.ts b/packages/browser/test/renderOnce.test.ts index 1e11171..2d85b2e 100644 --- a/packages/browser/test/renderOnce.test.ts +++ b/packages/browser/test/renderOnce.test.ts @@ -178,6 +178,44 @@ test('waitFor path scoping: a rule only runs when the URL path matches pathPatte assert.equal(await count('^/'), 5, 'rule runs when path matches'); }); +test('waitFor page-type scoping: a rule only runs for its listed page types', async () => { + // The point of scoping by NAME rather than by a path regex: the template comes from the + // plugin's route list, so this rule says "the pages that have this widget" in the same + // vocabulary that decided the page would be rendered at all. + const rule = (pageTypes: string[]) => ({ + selector: '#reviews', + waitForSelector: '.rev-item', + minCount: 1, + timeoutMs: 3000, + pageTypes, + }); + const count = async (pageTypes: string[], pageType?: string) => { + const r = await renderOnce({ + url: base, + device: 'mobile', + pageType, + config: { ...NO_SCROLL, waitFor: [rule(pageTypes)] }, + probes: { rev: selectorCountProbe(['.rev-item']) }, + }); + return (r.probes.rev as Record)['.rev-item']; + }; + assert.equal(await count(['pdp'], 'pdp'), 5, 'rule runs for its page type'); + assert.equal(await count(['pdp'], 'category'), 0, 'rule is skipped for another page type'); + // One template, several names — the multi-URL-shape case a regex would need an alternation for. + assert.equal(await count(['pdp', 'category'], 'category'), 5, 'matches any listed type'); + // A job with no declared type never matches: a plugin older than prerender-v0.34.0, or a route + // naming no template. Skipping costs this rule's content; applying it blindly would cost a poll + // to the timeout on every page lacking the widget, which is what the scoping exists to prevent. + assert.equal(await count(['pdp'], undefined), 0, 'a job with no page type matches no pageTypes rule'); +}); + +test('the job carries the page type through to the renderer', async () => { + const r = await renderOnce({ url: base, device: 'desktop', pageType: 'pdp', config: NO_SCROLL }); + assert.equal(r.job.pageType, 'pdp'); + const untyped = await renderOnce({ url: base, device: 'desktop', config: NO_SCROLL }); + assert.equal(untyped.job.pageType, undefined); +}); + test('captureNonIndexable returns HTML for a noindex page (and marks it non-indexable)', async () => { const r = await renderOnce({ url: `${base}/noindex`, device: 'desktop', config: NO_SCROLL }); assert.equal(r.isIndexable, false);