feat: migrate js to ts#15
Open
quanvu-optimizely wants to merge 7 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR converts BackstopJS engine scripts from CommonJS to TypeScript ES modules with comprehensive type safety. New type definitions establish contracts for scenarios, actions, and lifecycle callbacks. Parallel Playwright and Puppeteer implementations are converted separately, updating module exports, adding parameter/return type annotations, and clarifying interactions with DOM casting and Playwright/Puppeteer APIs. Build configuration is extended to compile engine TypeScript and apply post-build ESM interop fixes. ChangesEngine Script Migration to TypeScript/ESM
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.engine_scripts/puppet/overrideCSS.ts (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid interpolating raw CSS into evaluated JS string.
Line 18 is fragile: CSS containing quotes/newlines can break the evaluated script. Pass the payload as an argument instead.
Suggested fix
- await page.evaluate(`window._styleData = '${override}'`); + await page.evaluate((css: string) => { + window._styleData = css; + }, override);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/puppet/overrideCSS.ts at line 18, The current page.evaluate call interpolates the raw CSS string (override) into a template literal which breaks on quotes/newlines; change the page.evaluate usage to pass override as an argument (e.g., page.evaluate((css) => { window._styleData = css }, override)) so the CSS is not injected into source text, and ensure the assignment targets window._styleData and handles the passed-in value safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.engine_scripts/playwright/actions.ts:
- Around line 121-125: The remove-action branch is querying the wrong selector:
after calling page.waitForSelector(action.remove) the code uses
page.locator(action.hide) and hides the wrong element; change the locator to
page.locator(action.remove) (and then call el.evaluate as before) so the
selector used in waitForSelector and locator match the intended action.remove;
update any related variable uses (action.remove vs action.hide) in the remove
branch only.
- Around line 111-114: The current code awaits a 'filechooser' on (page as Page)
which can be a Frame at runtime (page is reassigned via handle.contentFrame()),
causing a runtime error; instead call waitForEvent('filechooser') on the root
Page (currentPage) and keep the frame-local locator (el) for the click only.
Change the sequence so you create fileChooserPromise on
currentPage.waitForEvent('filechooser'), then call el.click() on the frame-local
element, await the fileChooser, and call fileChooser.setFiles(normalizedPaths);
ensure you reference action.frame/handle.contentFrame() only for the locator and
not for waitForEvent.
- Around line 178-181: The current check using parseInt(String(action.wait)) > 0
can misclassify values like "1000ms" and then pass a non-number into
page.waitForTimeout; update the logic around action.wait so that you explicitly
detect numeric timeouts (either a number or a numeric string): if action.wait is
a number or a string that parses to a finite non-NaN integer (including 0),
convert it with Number.parseInt/Number() and call page.waitForTimeout with that
numeric value; otherwise treat action.wait as a selector string and call
page.waitForSelector. Ensure you guard against NaN and preserve zero (0) as a
valid timeout for page.waitForTimeout, referencing action.wait,
page.waitForTimeout, page.waitForSelector and the current parseInt usage for
locating the change.
- Around line 31-33: The code assumes page.locator(frames[j]).elementHandle()
and handle.contentFrame() are non-null; change the logic in the loop that calls
page.locator(...).elementHandle() and handle.contentFrame() to explicitly guard
both results: check the value returned by elementHandle() and if null throw or
return a descriptive Error (e.g., "iframe element not found for selector ..."),
then call handle.contentFrame() and if that is null also throw/return a clear
Error (e.g., "contentFrame missing for iframe element ..."); update any type
assertions (the as ElementHandle<...> and the trailing !) to use runtime-checked
values so you never dereference a potentially null handle or frame.
In @.engine_scripts/playwright/clickAndHoverHelper.ts:
- Around line 33-37: postInteractionWait is a number (ms) in the types/schemas,
so remove the parseInt(String(...)) logic and branch correctly: in
clickAndHoverHelper.ts check if postInteractionWait is a number and > 0 and call
page.waitForTimeout(postInteractionWait); only call page.waitForSelector when
postInteractionWait is actually a non-empty string (e.g., typeof
postInteractionWait === 'string' && postInteractionWait) and otherwise do
nothing; reference the postInteractionWait variable and the use of
page.waitForTimeout/page.waitForSelector to locate the change.
In @.engine_scripts/playwright/interceptImages.ts:
- Line 21: The route installation in interceptImages.ts is racing because
page.route(...) returns a Promise that isn’t awaited; update the interceptImages
function to await the page.route(...) call so the route is installed before the
function resolves (reference the page.route call and IMAGE_STUB_URL constant),
and leave the __dirname usage as-is since this script runs under CommonJS in
.engine_scripts so no change is needed there.
In @.engine_scripts/playwright/onReady.ts:
- Around line 45-47: onReady.ts currently always calls
page.waitForTimeout(scenario.postInteractionWait as number), which breaks when
postInteractionWait is a selector string; update the post-interaction logic in
the onReady handler to check the type/value of scenario.postInteractionWait and,
if it's a number, call page.waitForTimeout(...) but if it's a string call
page.waitForSelector(...) (matching the behavior in clickAndHoverHelper.ts).
Locate the code that references scenario.postInteractionWait in onReady.ts and
replace the unconditional cast/use with a type/typeof check (or truthy string
test) and the two appropriate Playwright calls to support both number and
selector string cases. Ensure the change aligns with the engine.d.ts union type
(number | string).
- Around line 16-17: The file uses CommonJS require calls that break in ESM;
replace each require(...) usage with dynamic ESM imports inside the async
onReady flow—for example use const { default: EmbedFiles } = await
import('./embedFiles') (or const mod = await import('./embedFiles'); await
mod.default(scenario, page)) instead of require('./embedFiles') as EmbedFiles,
and similarly replace require('../auto-scroll') with const { default: autoScroll
} = await import('../auto-scroll'), require('./actions') with await
import('./actions') (extract the named exports you need), and
require('./clickAndHoverHelper') with await import('./clickAndHoverHelper');
ensure you call the imported defaults/named exports the same way as before and
keep these awaits inside the async function so top-level await isn’t required.
In @.engine_scripts/puppet/clickAndHoverHelper.ts:
- Around line 32-34: The current block always treats postInteractionWait as
milliseconds and loses selector-based waits; change the logic in
clickAndHoverHelper.ts to branch on the type/value of postInteractionWait: if
it's a string treat it as a selector and await page.waitForSelector(selector),
if it's a number treat it as milliseconds and await page.waitForTimeout(ms); use
the same page cast used currently (or Playwright Page) so both waitForSelector
and waitForTimeout are available and retain existing behavior when
postInteractionWait is undefined/null.
In @.engine_scripts/puppet/ignoreCSP.ts:
- Around line 81-83: The page.on('request', ...) callback calls intercept(req,
scenario.url!) without handling its returned Promise, risking unhandled
rejections; update the handler to handle async errors by either making the
callback async and awaiting intercept(...) inside a try/catch or calling
intercept(...).catch(...) and logging or recovering on error (referencing
page.on, intercept, and scenario.url!). Ensure any failure path explicitly
continues/aborts the request or logs the error so requests do not hang.
In @.engine_scripts/puppet/loadCookies.ts:
- Around line 24-27: The code assumes c.domain exists before iterating and will
throw in [].forEach.call(domains, ...) when it's missing; update the logic in
loadCookies.ts around the domains assignment (variable domains and the iteration
using [].forEach.call(domains, ...)) to first guard for c.domain (e.g., if
(!c.domain) set domains = [] or skip processing this cookie), or normalize
c.domain into a safe array only when defined, so the loop never receives
undefined and cookie restoration continues safely.
In @.engine_scripts/puppet/onBefore.ts:
- Line 6: The call is passing browserContext (cast to Page) into the cookie
loader which expects a real Page and uses setCookie; update the call to pass the
actual Page object (named page) instead of browserContext and remove the unsafe
cast: invoke loadCookies via (require('./loadCookies') as
PuppetLoadCookies)(page, scenario) so the loader receives a proper Page
supporting setCookie (symbols: loadCookies, PuppetLoadCookies, browserContext,
page, setCookie).
In @.engine_scripts/puppet/onReady.ts:
- Around line 16-21: The current logic returns early when jsOnReadyPath is
missing, skipping common post-ready handlers; change the flow so that existence
checks only gate the JS injection (jsOnReadyPath and
fs.existsSync(jsOnReadyPath)) but do not return from the function—always
continue to invoke the common handlers like autoScroll() and
waitForNetworkIdle() after attempting conditional injection; apply the same
change to the second similar block (the other jsOnReadyPath check around the
lines referenced) so both places perform JS injection only when present and then
proceed to the shared post-ready steps.
---
Outside diff comments:
In @.engine_scripts/puppet/overrideCSS.ts:
- Line 18: The current page.evaluate call interpolates the raw CSS string
(override) into a template literal which breaks on quotes/newlines; change the
page.evaluate usage to pass override as an argument (e.g., page.evaluate((css)
=> { window._styleData = css }, override)) so the CSS is not injected into
source text, and ensure the assignment targets window._styleData and handles the
passed-in value safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 707cc35a-5684-4bad-9787-42c25994b6cb
📒 Files selected for processing (27)
.engine_scripts/auto-scroll.ts.engine_scripts/engine.d.ts.engine_scripts/engine.globals.d.ts.engine_scripts/playwright/actions.ts.engine_scripts/playwright/clickAndHoverHelper.ts.engine_scripts/playwright/embedFiles.ts.engine_scripts/playwright/interceptImages.ts.engine_scripts/playwright/loadCookies.ts.engine_scripts/playwright/onBefore.ts.engine_scripts/playwright/onReady.js.engine_scripts/playwright/onReady.ts.engine_scripts/playwright/overrideCSS.ts.engine_scripts/puppet/clickAndHoverHelper.ts.engine_scripts/puppet/ignoreCSP.ts.engine_scripts/puppet/interceptImages.ts.engine_scripts/puppet/loadCookies.js.engine_scripts/puppet/loadCookies.ts.engine_scripts/puppet/onBefore.js.engine_scripts/puppet/onBefore.ts.engine_scripts/puppet/onReady.js.engine_scripts/puppet/onReady.ts.engine_scripts/puppet/overrideCSS.ts.engine_scripts/scroll-top.ts.gitignorepackage.jsonscripts/fix-engine-exports.tstsconfig.engine.json
💤 Files with no reviewable changes (4)
- .engine_scripts/playwright/onReady.js
- .engine_scripts/puppet/loadCookies.js
- .engine_scripts/puppet/onReady.js
- .engine_scripts/puppet/onBefore.js
There was a problem hiding this comment.
Pull request overview
Migrates the BackstopJS engine scripts under .engine_scripts/ from CommonJS JavaScript to TypeScript, adding a dedicated engine build pipeline and a post-build step to preserve BackstopJS’s expected require()/function-export behavior.
Changes:
- Added
tsconfig.engine.jsonand updatedpackage.jsonscripts to compile engine scripts separately. - Converted engine scripts (Puppeteer + Playwright helpers) from
module.exportsto typedexport defaultimplementations. - Added a post-build patch script (
scripts/fix-engine-exports.ts) and updated.gitignoreto avoid committing generated engine.js/.mapartifacts.
Reviewed changes
Copilot reviewed 24 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tsconfig.engine.json |
New TS build config for compiling .engine_scripts sources. |
scripts/fix-engine-exports.ts |
Post-build step to unwrap default exports for BackstopJS CommonJS loading. |
package.json |
Updates build/typecheck scripts to include engine compilation + post-build patch. |
.gitignore |
Ignores compiled .engine_scripts/**/*.js and sourcemaps. |
.engine_scripts/auto-scroll.ts |
Migrated to TS + default export. |
.engine_scripts/scroll-top.ts |
Migrated to TS + default export and typed promises. |
.engine_scripts/engine.d.ts |
Adds shared TS types for engine scripts (scenario/actions/contracts). |
.engine_scripts/engine.globals.d.ts |
Adds global Window augmentations used by browser-evaluated scripts. |
.engine_scripts/puppet/clickAndHoverHelper.ts |
Migrated to TS; adds typings for scenario-driven interactions. |
.engine_scripts/puppet/ignoreCSP.ts |
Migrated to TS; adds typings around fetch/interception logic. |
.engine_scripts/puppet/interceptImages.ts |
Migrated to TS; adds Puppeteer types and minor formatting changes. |
.engine_scripts/puppet/loadCookies.ts |
New TS version of cookie-loading helper (replaces JS). |
.engine_scripts/puppet/onBefore.ts |
New TS version of Puppeteer onBefore script (replaces JS). |
.engine_scripts/puppet/onReady.ts |
New TS version of Puppeteer onReady script (replaces JS). |
.engine_scripts/puppet/overrideCSS.ts |
Migrated to TS; adds types and non-null assertion for injected style data. |
.engine_scripts/puppet/onBefore.js |
Removes legacy JS version (now generated via build). |
.engine_scripts/puppet/onReady.js |
Removes legacy JS version (now generated via build). |
.engine_scripts/puppet/loadCookies.js |
Removes legacy JS version (now generated via build). |
.engine_scripts/playwright/actions.ts |
Migrated to TS; adds types for scenario actions and frame handling. |
.engine_scripts/playwright/clickAndHoverHelper.ts |
Migrated to TS; preserves selector-vs-timeout handling. |
.engine_scripts/playwright/embedFiles.ts |
Migrated to TS; adds typed embedding of JS/CSS helpers. |
.engine_scripts/playwright/interceptImages.ts |
Migrated to TS; adds Playwright types and minor formatting changes. |
.engine_scripts/playwright/loadCookies.ts |
Migrated to TS; adds typed cookie parsing and addCookies call. |
.engine_scripts/playwright/onBefore.ts |
Migrated to TS; adds Playwright typings and CSP bypass logic typing. |
.engine_scripts/playwright/onReady.ts |
New TS version of Playwright onReady script (replaces JS). |
.engine_scripts/playwright/overrideCSS.ts |
Migrated to TS; uses addStyleTag({ path }) for CSS injection. |
.engine_scripts/playwright/onReady.js |
Removes legacy JS version (now generated via build). |
Comments suppressed due to low confidence (4)
.engine_scripts/puppet/clickAndHoverHelper.ts:34
postInteractionWaitpreviously supported either a selector string or a millisecond timeout. The updated implementation always callswaitForTimeout(postInteractionWait as number), which will break the selector use-case (and can passNaNat runtime). Restore the selector-vs-timeout branching (like the Playwright helper does) and avoid the unsafe casts.
.engine_scripts/puppet/ignoreCSP.ts:44- This script still
require()snode-fetch, butnode-fetchis not present inpackage.json(and doesn’t appear inpackage-lock.json). On Node 24 (see.node-version), you can use the built-in globalfetchinstead, or addnode-fetchas a direct dependency if you need its specific API surface.
.engine_scripts/playwright/actions.ts:126 - In the
action.removeblock, the code waits foraction.removebut then callspage.locator(action.hide as string). Ifhideisn’t set, this will throw at runtime; and even if it is set, it’s inconsistent with the logged selector. Useaction.removefor the locator (or explicitly validate/rename the fields if both are required).
.engine_scripts/playwright/overrideCSS.ts:25 - The unconditional
console.log('cssOverridePath', cssOverridePath);looks like leftover debug output and will add noise to normal runs (and may leak local paths). Consider removing it or gating it behind an existing debug flag/env var.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
.engine_scripts/playwright/actions.ts (4)
120-125:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
removeaction queries wrong selector (action.hideinstead ofaction.remove).Line 123 locates
action.hidebut should locateaction.remove. This causes the remove action to target the wrong element or fail.Proposed fix
if (!!action.remove) { console.log(logPrefix + 'Remove:', action.remove); await page.waitForSelector(action.remove); - let el = await page.locator(action.hide as string); + let el = await page.locator(action.remove); await el.evaluate((node) => node.style.setProperty('display', 'none', 'important')); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/playwright/actions.ts around lines 120 - 125, The remove-action block incorrectly locates action.hide instead of action.remove; update the branch handling the remove action (the if (!!action.remove) block) so that after waiting for the selector it calls page.locator(action.remove) (and then hides/removes that locator) rather than page.locator(action.hide), ensuring the locator and evaluate call target the element identified by action.remove.
109-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
waitForEvent('filechooser')will fail whenpageis aFrame.When
action.frameis set,pageis reassigned to aFrame(line 31). The code then calls(page as Page).waitForEvent('filechooser')on line 110, butFramedoesn't havewaitForEvent. Use the originalcurrentPagefor the filechooser event.Proposed fix
if (action.useFileChooser) { - const fileChooserPromise = (page as Page).waitForEvent('filechooser'); + const fileChooserPromise = currentPage.waitForEvent('filechooser'); el.click(); const fileChooser = await fileChooserPromise; await fileChooser.setFiles(normalizedPaths);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/playwright/actions.ts around lines 109 - 116, The filechooser wait is invoked on a variable that may be a Frame (variable page) causing Frame.waitForEvent to be missing; update the file-choose branch in the handler for action.useFileChooser to use the original Page object (currentPage) to call waitForEvent('filechooser') instead of (page as Page).waitForEvent, keep el.click() and await the returned fileChooser before calling setFiles(normalizedPaths), and ensure you reference currentPage so the event is always awaited on a Page, not a Frame.
30-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNull-safety issue persists:
elementHandle()andcontentFrame()can return null.The non-null assertion (
!) and type cast on lines 30-31 remain unchanged. BothelementHandle()andcontentFrame()can returnnull, causing a runtime crash if the iframe selector doesn't match or the element isn't an iframe.Proposed fix
for (let j = 0; j < frames.length; j++) { await page.waitForSelector(frames[j]); - const handle = (await page.locator(frames[j]).elementHandle()) as ElementHandle<SVGElement | HTMLElement>; - page = (await handle.contentFrame())!; + const handle = await page.locator(frames[j]).elementHandle(); + if (!handle) { + throw new Error(`Could not get element handle for iframe selector: ${frames[j]}`); + } + const frame = await handle.contentFrame(); + if (!frame) { + throw new Error(`Could not get content frame for iframe selector: ${frames[j]}`); + } + page = frame; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/playwright/actions.ts around lines 30 - 32, The code assumes elementHandle() and contentFrame() never return null; update the logic around the ElementHandle retrieval and frame switch so you null-check the result of page.locator(frames[j]).elementHandle() (the variable handle) and the result of handle.contentFrame() before casting or using non-null assertions. If elementHandle() is null, throw or return a clear error indicating the iframe selector failed; if contentFrame() is null, handle it similarly (skip, throw, or return an error) instead of using the ! cast—adjust the surrounding function to propagate or handle these error cases (references: the local variable handle, the call to elementHandle(), and the call to contentFrame()).
177-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
action.waithandling still passes original value towaitForTimeout.The added
!Number.isNaN(...)check is redundant (ifparseInt > 0, it's already not NaN). More importantly, the code still passesaction.wait as numbertowaitForTimeout, so ifaction.waitis a string like"1000ms",parseIntreturns1000(passes the check), butwaitForTimeoutreceives the string"1000ms"which coerces toNaNat runtime.Proposed fix
- if (parseInt(String(action.wait)) > 0 && !Number.isNaN(parseInt(String(action.wait)))) { - await page.waitForTimeout(action.wait as number); + const parsedWait = parseInt(String(action.wait), 10); + if (!Number.isNaN(parsedWait) && parsedWait >= 0) { + await page.waitForTimeout(parsedWait); } else { await page.waitForSelector(action.wait as string); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/playwright/actions.ts around lines 177 - 181, The current branch tests parseInt(String(action.wait)) but still passes action.wait directly to page.waitForTimeout, which fails when action.wait is a numeric string like "1000ms"; fix by parsing once and using the parsed numeric value: compute const timeout = parseInt(String(action.wait)) and if timeout > 0 call await page.waitForTimeout(timeout) else call await page.waitForSelector(String(action.wait)); remove the redundant Number.isNaN check and ensure you cast selector arguments to string when calling page.waitForSelector..engine_scripts/playwright/onReady.ts (2)
16-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winESM breakage:
require()is not available in ESM modules.This file uses
require()calls (lines 16, 23-24, 33, 35) but the project is configured for ESM ("type": "module",module: "node16"). These will fail at runtime. Convert to dynamicimport().Proposed fix (example for line 16)
- await (require('./embedFiles') as EmbedFiles)(scenario, page); + const { default: embedFiles } = await import('./embedFiles.js'); + await (embedFiles as EmbedFiles)(scenario, page);Apply similar changes for lines 23-24, 33, and 35.
Also applies to: 23-24, 33-33, 35-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/playwright/onReady.ts around lines 16 - 17, This file uses CommonJS require() which breaks under ESM; replace all require(...) calls (e.g., require('./embedFiles') used with EmbedFiles on the await line, and the other requires at the indicated sites on lines 23-24, 33 and 35) with dynamic import() calls and invoke the exported value appropriately (await (await import('./embedFiles')).default or the named export as used) so the module loads in ESM; ensure you preserve the same call shape (casting to EmbedFiles or calling the exported function) and update any destructuring/usage if the module exports differ between default and named exports.
45-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
postInteractionWaittype mismatch:waitForTimeoutexpectsnumber, but value can bestring.The
as numbercast was removed, butscenario.postInteractionWaitis typed asnumber | stringperengine.d.ts. Passing a string towaitForTimeout(number)will cause unexpected behavior. Add type checking to handle both cases consistently withclickAndHoverHelper.ts.Proposed fix
if (scenario.postInteractionWait) { - await page.waitForTimeout(scenario.postInteractionWait); + if (typeof scenario.postInteractionWait === 'number') { + await page.waitForTimeout(scenario.postInteractionWait); + } else { + await page.waitForSelector(scenario.postInteractionWait); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/playwright/onReady.ts around lines 45 - 47, scenario.postInteractionWait is typed number | string but page.waitForTimeout requires a number; add runtime type handling similar to clickAndHoverHelper.ts: check typeof scenario.postInteractionWait, if it's a string convert to a numeric value (e.g., Number(...) or parseInt with a fallback) and validate it's finite, otherwise use the numeric value directly; only call await page.waitForTimeout with a valid number (or skip the wait if conversion fails/<=0) so page.waitForTimeout always receives a number..engine_scripts/puppet/ignoreCSP.ts (1)
81-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle rejected interceptions in the request listener.
Line 81/82 awaits async interception without failure handling; rejected interception can become an unhandled rejection and leave request handling unstable.
Suggested fix
- page.on('request', async (req) => { - await intercept(req, scenario.url); - }); + page.on('request', (req) => { + void intercept(req, scenario.url).catch((err) => { + console.error('ignoreCSP interception failed:', err); + void req.continue().catch(() => undefined); + }); + });#!/bin/bash # Verify async request listeners and interception error handling in puppet helpers rg -n -C3 "page\\.on\\('request'" .engine_scripts/puppet/ignoreCSP.ts rg -n -C3 "intercept\\(req,\\s*scenario\\.url\\)" .engine_scripts/puppet/ignoreCSP.ts rg -n -C3 "\\.catch\\(" .engine_scripts/puppet/ignoreCSP.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/puppet/ignoreCSP.ts around lines 81 - 82, The page.request listener awaits intercept(req, scenario.url) without handling promise rejections; update the listener registered on page.on('request', ...) to catch errors from intercept(req, scenario.url) (either wrap the await in try/catch or append .catch()) and ensure the request is always resolved (e.g., call the appropriate continuation/abort fallback inside the catch) so rejected interceptions cannot produce unhandled rejections or leave requests hanging; locate the handler that calls intercept(req, scenario.url) and add the error handling around that call..engine_scripts/puppet/onBefore.ts (1)
5-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the actual
pageto cookie loading.Line 6 should use
pageinstead ofbrowserContext; this keeps the loader bound to a concrete PuppeteerPageAPI and avoids runtime mismatches in the hook’s 5th argument.Suggested fix
export default (async (page: Page, scenario: EngineScenario, viewport: Viewport, isReference: boolean, browserContext: Page): Promise<void> => { - await (require('./loadCookies') as PuppetLoadCookies)(browserContext, scenario); + await (require('./loadCookies') as PuppetLoadCookies)(page, scenario); }) as PuppetOnBeforeScript;#!/bin/bash # Verify hook/cookie loader contracts and current call site usage rg -n -C3 "type\\s+PuppetOnBeforeScript|type\\s+PuppetLoadCookies|interface\\s+EngineScenario" .engine_scripts/engine.d.ts .engine_scripts/engine.d.ts rg -n -C3 "export default \\(async \\(page: Page, scenario: EngineScenario, viewport: Viewport, isReference: boolean, browserContext" .engine_scripts/puppet/onBefore.ts rg -n -C3 "loadCookies\\) as PuppetLoadCookies\\)\\(" .engine_scripts/puppet/onBefore.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/puppet/onBefore.ts around lines 5 - 6, The onBefore hook is passing browserContext to the cookie loader instead of the Puppeteer Page, causing a type/runtime mismatch; update the call inside the exported async function so the loadCookies invocation (cast as PuppetLoadCookies) receives the page parameter (named page) rather than browserContext so the loader uses the concrete Page API; ensure the call site referencing loadCookies/PuppetLoadCookies is changed accordingly and retains existing await/Promise handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.engine_scripts/engine.d.ts:
- Line 31: ScenarioAction currently requires url for every action (url: string
on ScenarioAction), which incorrectly rejects non-goto actions; change the type
to make url optional for all actions and/or refactor ScenarioAction into a
discriminated union (e.g., a { type: 'goto'; url: string; ... } branch and other
branches like { type: 'click'|'hover'|'wait'; ... } without url) so only the
'goto' variant requires url; update the ScenarioAction definition to use either
url?: string or the discriminated union pattern so non-goto actions no longer
need a url.
---
Duplicate comments:
In @.engine_scripts/playwright/actions.ts:
- Around line 120-125: The remove-action block incorrectly locates action.hide
instead of action.remove; update the branch handling the remove action (the if
(!!action.remove) block) so that after waiting for the selector it calls
page.locator(action.remove) (and then hides/removes that locator) rather than
page.locator(action.hide), ensuring the locator and evaluate call target the
element identified by action.remove.
- Around line 109-116: The filechooser wait is invoked on a variable that may be
a Frame (variable page) causing Frame.waitForEvent to be missing; update the
file-choose branch in the handler for action.useFileChooser to use the original
Page object (currentPage) to call waitForEvent('filechooser') instead of (page
as Page).waitForEvent, keep el.click() and await the returned fileChooser before
calling setFiles(normalizedPaths), and ensure you reference currentPage so the
event is always awaited on a Page, not a Frame.
- Around line 30-32: The code assumes elementHandle() and contentFrame() never
return null; update the logic around the ElementHandle retrieval and frame
switch so you null-check the result of page.locator(frames[j]).elementHandle()
(the variable handle) and the result of handle.contentFrame() before casting or
using non-null assertions. If elementHandle() is null, throw or return a clear
error indicating the iframe selector failed; if contentFrame() is null, handle
it similarly (skip, throw, or return an error) instead of using the !
cast—adjust the surrounding function to propagate or handle these error cases
(references: the local variable handle, the call to elementHandle(), and the
call to contentFrame()).
- Around line 177-181: The current branch tests parseInt(String(action.wait))
but still passes action.wait directly to page.waitForTimeout, which fails when
action.wait is a numeric string like "1000ms"; fix by parsing once and using the
parsed numeric value: compute const timeout = parseInt(String(action.wait)) and
if timeout > 0 call await page.waitForTimeout(timeout) else call await
page.waitForSelector(String(action.wait)); remove the redundant Number.isNaN
check and ensure you cast selector arguments to string when calling
page.waitForSelector.
In @.engine_scripts/playwright/onReady.ts:
- Around line 16-17: This file uses CommonJS require() which breaks under ESM;
replace all require(...) calls (e.g., require('./embedFiles') used with
EmbedFiles on the await line, and the other requires at the indicated sites on
lines 23-24, 33 and 35) with dynamic import() calls and invoke the exported
value appropriately (await (await import('./embedFiles')).default or the named
export as used) so the module loads in ESM; ensure you preserve the same call
shape (casting to EmbedFiles or calling the exported function) and update any
destructuring/usage if the module exports differ between default and named
exports.
- Around line 45-47: scenario.postInteractionWait is typed number | string but
page.waitForTimeout requires a number; add runtime type handling similar to
clickAndHoverHelper.ts: check typeof scenario.postInteractionWait, if it's a
string convert to a numeric value (e.g., Number(...) or parseInt with a
fallback) and validate it's finite, otherwise use the numeric value directly;
only call await page.waitForTimeout with a valid number (or skip the wait if
conversion fails/<=0) so page.waitForTimeout always receives a number.
In @.engine_scripts/puppet/ignoreCSP.ts:
- Around line 81-82: The page.request listener awaits intercept(req,
scenario.url) without handling promise rejections; update the listener
registered on page.on('request', ...) to catch errors from intercept(req,
scenario.url) (either wrap the await in try/catch or append .catch()) and ensure
the request is always resolved (e.g., call the appropriate continuation/abort
fallback inside the catch) so rejected interceptions cannot produce unhandled
rejections or leave requests hanging; locate the handler that calls
intercept(req, scenario.url) and add the error handling around that call.
In @.engine_scripts/puppet/onBefore.ts:
- Around line 5-6: The onBefore hook is passing browserContext to the cookie
loader instead of the Puppeteer Page, causing a type/runtime mismatch; update
the call inside the exported async function so the loadCookies invocation (cast
as PuppetLoadCookies) receives the page parameter (named page) rather than
browserContext so the loader uses the concrete Page API; ensure the call site
referencing loadCookies/PuppetLoadCookies is changed accordingly and retains
existing await/Promise handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d651265-1ca1-4aed-bcfe-006204a2dacd
📒 Files selected for processing (11)
.engine_scripts/engine.d.ts.engine_scripts/playwright/actions.ts.engine_scripts/playwright/clickAndHoverHelper.ts.engine_scripts/playwright/interceptImages.ts.engine_scripts/playwright/onReady.ts.engine_scripts/puppet/clickAndHoverHelper.ts.engine_scripts/puppet/ignoreCSP.ts.engine_scripts/puppet/loadCookies.ts.engine_scripts/puppet/onBefore.ts.engine_scripts/puppet/onReady.tstsconfig.engine.json
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.engine_scripts/playwright/clickAndHoverHelper.ts:
- Around line 33-37: The timeout uses the unparsed postInteractionWait instead
of the parsed interactionWait; update calls to page.waitForTimeout to pass the
parsed interactionWait variable (i.e., use interactionWait rather than
postInteractionWait as number) inside clickAndHoverHelper, and apply the same
fix in the onReady.ts and puppet/clickAndHoverHelper.ts implementations so the
parsed integer is actually used for the timeout check and wait.
In @.engine_scripts/playwright/onReady.ts:
- Around line 46-50: Normalize scenario.postInteractionWait into the parsed
numeric variable before calling waitForTimeout: use the already computed
interactionWait (parsed via parseInt/Number) and call
page.waitForTimeout(interactionWait) when it's a valid >0 number, otherwise call
page.waitForSelector with the original string; update the branch in onReady.ts
(the interactionWait and page.waitForTimeout/page.waitForSelector logic) to pass
interactionWait instead of scenario.postInteractionWait, and apply the identical
change in clickAndHoverHelper.ts so both locations consistently parse numeric
strings and treat "0" correctly.
In @.engine_scripts/puppet/clickAndHoverHelper.ts:
- Around line 33-37: The code incorrectly passes postInteractionWait to
waitForTimeout and may call waitForSelector with "0"/negative values; change the
logic in clickAndHoverHelper.ts to use the parsed interactionWait variable: if
Number.isNaN(interactionWait) === false and interactionWait > 0 then await
page.waitForTimeout(interactionWait); else if typeof postInteractionWait ===
'string' and Number.isNaN(parseInt(String(postInteractionWait))) then await
page.waitForSelector(postInteractionWait as string); otherwise skip waiting (do
not call waitForSelector with "0"/negative). Ensure you reference
interactionWait, postInteractionWait, waitForTimeout and waitForSelector when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0dbda58-6b03-46dd-a630-77c6fd840fe5
📒 Files selected for processing (5)
.engine_scripts/engine.d.ts.engine_scripts/playwright/actions.ts.engine_scripts/playwright/clickAndHoverHelper.ts.engine_scripts/playwright/onReady.ts.engine_scripts/puppet/clickAndHoverHelper.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.engine_scripts/playwright/onReady.ts (1)
45-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
postInteractionWait: 0still skips this branch.Line 45 still gates on truthiness, so a numeric
0never reaches the newinteractionWait >= 0path. That leaves the 0ms case broken here, and.engine_scripts/playwright/clickAndHoverHelper.tshas the same guard.Suggested fix
- if (scenario.postInteractionWait) { - const interactionWait = parseInt(String(scenario.postInteractionWait)); + if (scenario.postInteractionWait !== undefined && scenario.postInteractionWait !== null) { + const rawWait = scenario.postInteractionWait; + const interactionWait = parseInt(String(rawWait)); if (!Number.isNaN(interactionWait) && interactionWait >= 0) { await page.waitForTimeout(interactionWait); - } else { - await page.waitForSelector(scenario.postInteractionWait as string); + } else if (typeof rawWait === 'string' && rawWait) { + await page.waitForSelector(rawWait); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.engine_scripts/playwright/onReady.ts around lines 45 - 50, The guard currently uses truthiness on scenario.postInteractionWait so numeric 0 is skipped; change the condition to explicitly check for presence (e.g., if (scenario.postInteractionWait !== undefined && scenario.postInteractionWait !== null) or typeof scenario.postInteractionWait !== 'undefined') before parsing: inside that block keep parseInt(String(scenario.postInteractionWait)) and the Number.isNaN/interactionWait >= 0 path, and in the else branch use waitForSelector as today. Apply the same explicit-presence fix to the identical guard in .engine_scripts/playwright/clickAndHoverHelper.ts so 0ms is handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.engine_scripts/puppet/clickAndHoverHelper.ts:
- Around line 32-35: The truthy guard on postInteractionWait prevents numeric 0
from being processed; change the condition that currently reads if
(postInteractionWait) to a nullish check (e.g., if (postInteractionWait != null)
or if (postInteractionWait !== null && postInteractionWait !== undefined)) so
numeric 0 reaches the parseInt/interactionWait logic, then leave the existing
parseInt and Number.isNaN/>= 0 checks intact to call
page.waitForTimeout(interactionWait) when valid.
---
Duplicate comments:
In @.engine_scripts/playwright/onReady.ts:
- Around line 45-50: The guard currently uses truthiness on
scenario.postInteractionWait so numeric 0 is skipped; change the condition to
explicitly check for presence (e.g., if (scenario.postInteractionWait !==
undefined && scenario.postInteractionWait !== null) or typeof
scenario.postInteractionWait !== 'undefined') before parsing: inside that block
keep parseInt(String(scenario.postInteractionWait)) and the
Number.isNaN/interactionWait >= 0 path, and in the else branch use
waitForSelector as today. Apply the same explicit-presence fix to the identical
guard in .engine_scripts/playwright/clickAndHoverHelper.ts so 0ms is handled
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 883a3e64-1310-45d6-9d76-56befa951aa1
📒 Files selected for processing (3)
.engine_scripts/playwright/clickAndHoverHelper.ts.engine_scripts/playwright/onReady.ts.engine_scripts/puppet/clickAndHoverHelper.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
.engine_scripts/puppet/interceptImages.ts:27
- The request type extraction
Parameters<Parameters<typeof page.on<'request'>>[1]>[0]is invalid TypeScript syntax for Puppeteer’s genericpage.on(...)overloads and will fail to compile. Prefer explicitly importing Puppeteer’s request type (e.g.,HTTPRequest) or deriving it fromParameters<Page['on']>[1]without attemptingpage.on<'request'>.
.engine_scripts/puppet/ignoreCSP.ts:44 - This file
requiresnode-fetch, butnode-fetchis not listed inpackage.jsondependencies. This will break at runtime unless it happens to be provided transitively; either add it as a direct dependency or switch to the built-inglobalThis.fetchavailable in modern Node versions.
.engine_scripts/puppet/ignoreCSP.ts:50 - The request type extraction
Parameters<Parameters<typeof page.on<'request'>>[1]>[0]is not valid for Puppeteer event typings and will fail to compile. Use Puppeteer’sHTTPRequesttype (or another explicit request type) instead of trying to specializepage.onwith<'request'>.
.engine_scripts/puppet/ignoreCSP.ts:73 result.headers._headerscan containstring[]values (per this file’s ownFetchFntype), butrequest.respondexpects header values to be strings. Casting toRecord<string, string>can hide a real runtime failure; normalize array values (e.g., join with,or pick the first) before responding.
.engine_scripts/puppet/overrideCSS.ts:19- Interpolating raw CSS into a JS string (
page.evaluate(window._styleData = '${override}')) will break when the CSS contains quotes/newlines and can potentially allow script injection. Pass the CSS as an argument toevaluateor usepage.addStyleTag({ content: override })to avoid string interpolation.
.engine_scripts/playwright/overrideCSS.ts:25 - This unconditional
console.log('cssOverridePath', cssOverridePath)looks like leftover debug output and will add noise (and potentially leak local paths) on every scenario. Consider removing it or gating it behind a debug flag/env var.
.engine_scripts/playwright/actions.ts:123 Locator.setInputFiles(...)returns a promise; withoutawait, the script can continue before the files are actually set, which can break later steps. Addawaithere for deterministic behavior.
.engine_scripts/playwright/actions.ts:138action.key!is asserted non-null, butkeyis optional inScenarioAction. If a scenario definespresswithoutkey, this will fail at runtime with a less actionable error. Consider validatingaction.keywhenaction.pressis set and throwing a clear message (or makingkeyrequired forpressin the action type).
Contributor
|
Review finding from the JS-to-TS migration compatibility check: High: fresh checkout/package can no longer run Backstop engine hooks. The runtime config still points Backstop at generated I verified this with a clean This looks like a breaking change for fresh checkouts, git installs, publish flows, and consumers running the CLI without first building engine scripts. Suggested fixes: either keep committing the generated Validation I ran:
|
Collaborator
Author
Thank you! I updated a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.