-
Notifications
You must be signed in to change notification settings - Fork 3
Before hook playwright script #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -36,13 +36,15 @@ class Action { | |||||
| private expectation: string | null = null; | ||||||
| public lastError: Error | null = null; | ||||||
| public playwrightHelper: any; | ||||||
| private explorer: any; | ||||||
|
|
||||||
| constructor(actor: CodeceptJS.I, stateManager: StateManager) { | ||||||
| constructor(actor: CodeceptJS.I, stateManager: StateManager, explorer?: any) { | ||||||
| this.actor = actor; | ||||||
| this.stateManager = stateManager; | ||||||
| this.experienceTracker = stateManager.getExperienceTracker(); | ||||||
| this.config = ConfigParser.getInstance().getConfig(); | ||||||
| this.playwrightHelper = container.helpers('Playwright'); | ||||||
| this.explorer = explorer; | ||||||
| } | ||||||
|
|
||||||
| async caputrePageWithScreenshot(): Promise<ActionResult> { | ||||||
|
|
@@ -194,6 +196,10 @@ class Action { | |||||
| await recorder.add(() => sleep(this.config.action?.delay || 500)); // wait for the action to be executed | ||||||
| await recorder.promise(); | ||||||
|
|
||||||
| if (this.explorer?.waitForPendingSetupScripts) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The call to Severity Level: Major
|
||||||
| if (this.explorer?.waitForPendingSetupScripts) { | |
| if (this.explorer && typeof this.explorer.waitForPendingSetupScripts === 'function') { |
Steps of Reproduction ✅
1. Construct an Action with a non-function explorer value:
- In any script or test instantiate Action as `new Action(actor, stateManager,
explorer)` where the constructor is defined at `src/action.ts:41-48`
(`constructor(actor: CodeceptJS.I, stateManager: StateManager, explorer?: any)`).
2. Pass a mis-shaped explorer object:
- Provide `explorer = { waitForPendingSetupScripts: true }` (boolean) or any
non-callable value for that property when creating the Action instance.
3. Execute an action that runs the normal execute flow:
- Call `await action.execute("I.click('selector')")`. The `execute` function flow is in
`src/action.ts` around lines 186-206 (where `codeFunction` is invoked, `recorder.add()`
and `recorder.promise()` are awaited).
4. Observe the runtime TypeError immediately after navigation wait:
- After `await recorder.promise()` completes, the code at `src/action.ts:199-201` runs:
- `199 if (this.explorer?.waitForPendingSetupScripts) {`
- `200 await this.explorer.waitForPendingSetupScripts();`
- `201 }`
- Because `this.explorer.waitForPendingSetupScripts` is not a function (it's boolean),
awaiting it throws `TypeError: this.explorer.waitForPendingSetupScripts is not a
function`.
- The TypeError prevents the subsequent `capturePageState()` call at
`src/action.ts:203` and aborts the action.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/action.ts
**Line:** 199:199
**Comment:**
*Possible Bug: The call to `waitForPendingSetupScripts` on the loosely-typed `explorer` object only checks that the property exists, not that it is actually a function, so if a non-conforming object is ever passed in as `explorer` (for example via a misconfigured caller), this will throw a `TypeError: this.explorer.waitForPendingSetupScripts is not a function` at runtime and break the whole action execution.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import type { ExplorbotConfig } from './config.js'; | |||||||||||||||||||||||||
| import { ConfigParser } from './config.js'; | ||||||||||||||||||||||||||
| import type { UserResolveFunction } from './explorbot.js'; | ||||||||||||||||||||||||||
| import { KnowledgeTracker } from './knowledge-tracker.js'; | ||||||||||||||||||||||||||
| import { PageSetupManager } from './page-setup-manager.js'; | ||||||||||||||||||||||||||
| import { Reporter } from './reporter.ts'; | ||||||||||||||||||||||||||
| import { StateManager } from './state-manager.js'; | ||||||||||||||||||||||||||
| import { Test } from './test-plan.ts'; | ||||||||||||||||||||||||||
|
|
@@ -47,6 +48,8 @@ class Explorer { | |||||||||||||||||||||||||
| private options?: { show?: boolean; headless?: boolean; incognito?: boolean }; | ||||||||||||||||||||||||||
| private reporter!: Reporter; | ||||||||||||||||||||||||||
| private otherTabs: TabInfo[] = []; | ||||||||||||||||||||||||||
| private pageSetupManager!: PageSetupManager; | ||||||||||||||||||||||||||
| private pendingSetupScripts: Promise<void> | null = null; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| constructor(config: ExplorbotConfig, aiProvider: AIProvider, options?: { show?: boolean; headless?: boolean; incognito?: boolean }) { | ||||||||||||||||||||||||||
| this.config = config; | ||||||||||||||||||||||||||
|
|
@@ -56,6 +59,7 @@ class Explorer { | |||||||||||||||||||||||||
| this.stateManager = new StateManager({ incognito: this.options?.incognito }); | ||||||||||||||||||||||||||
| this.knowledgeTracker = new KnowledgeTracker(); | ||||||||||||||||||||||||||
| this.reporter = new Reporter(); | ||||||||||||||||||||||||||
| this.pageSetupManager = new PageSetupManager(this, config); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private initializeContainer() { | ||||||||||||||||||||||||||
|
|
@@ -180,6 +184,9 @@ class Explorer { | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| await this.playwrightHelper._startBrowser(); | ||||||||||||||||||||||||||
| await this.playwrightHelper._createContextPage(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| await this.pageSetupManager.loadSetupScripts(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const I = codeceptjs.container.support('I'); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.actor = I; | ||||||||||||||||||||||||||
|
|
@@ -195,7 +202,13 @@ class Explorer { | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| createAction() { | ||||||||||||||||||||||||||
| return new Action(this.actor, this.stateManager); | ||||||||||||||||||||||||||
| return new Action(this.actor, this.stateManager, this); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| async waitForPendingSetupScripts(): Promise<void> { | ||||||||||||||||||||||||||
| if (this.pendingSetupScripts) { | ||||||||||||||||||||||||||
| await this.pendingSetupScripts; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| async visit(url: string) { | ||||||||||||||||||||||||||
|
|
@@ -224,6 +237,8 @@ class Explorer { | |||||||||||||||||||||||||
| await action.execute(`I.waitForElement(${JSON.stringify(waitForElement)})`); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| await this.pageSetupManager.executeAfterNavigation(this.playwrightHelper.page, url); | ||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: After adding the Severity Level: Critical 🚨- ❌ Duplicate setup runs cause double form submissions.
- ❌ Cookie-banner dismissal may run twice unexpectedly.
- ⚠️ Explorer.visit flows may produce flaky captures.
Suggested change
Steps of Reproduction ✅1. Call Explorer.visit(url) (src/explorer.ts visit() function, see the line that executes
await this.pageSetupManager.executeAfterNavigation(this.playwrightHelper.page, url) at
line 240). visit() issues I.amOnPage(...) which triggers navigation.
2. The page navigation emits Playwright's 'framenavigated' event; the listener registered
in listenToStateChanged (src/explorer.ts framenavigated handler at lines ~328-337) also
calls pageSetupManager.executeAfterNavigation(...) for the same navigation.
3. Because visit() calls executeAfterNavigation directly (line 240) while the
framenavigated handler also calls it, the same setup scripts run twice for a single
navigation (PageSetupManager.executeAfterNavigation is invoked from both locations).
4. Observe duplicated side effects when running a non-idempotent setup script (e.g.,
browser-scripts that fill-and-submit login forms or click cookie banners) leading to
double submits or unexpected page state. The duplicate invocation originates from the two
call sites above.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/explorer.ts
**Line:** 240:240
**Comment:**
*Logic Error: After adding the `framenavigated` listener that already invokes `executeAfterNavigation`, the explicit call to `pageSetupManager.executeAfterNavigation` inside `visit()` causes setup scripts to run twice for navigations triggered via `visit()`, which can double-submit forms or perform other non-idempotent actions and lead to inconsistent browser state. Instead, `visit()` should only wait for any pending setup started by the navigation listener.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return action; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -313,10 +328,13 @@ class Explorer { | |||||||||||||||||||||||||
| debugLog('Failed to get page title:', error); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // // Update state from navigation | ||||||||||||||||||||||||||
| this.stateManager.updateStateFromBasic(newUrl, newTitle, 'navigation'); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.pendingSetupScripts = this.pageSetupManager.executeAfterNavigation(page, newUrl); | ||||||||||||||||||||||||||
| await this.pendingSetupScripts; | ||||||||||||||||||||||||||
| this.pendingSetupScripts = null; | ||||||||||||||||||||||||||
|
Comment on lines
+335
to
+337
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Using the shared Severity Level: Major
|
||||||||||||||||||||||||||
| this.pendingSetupScripts = this.pageSetupManager.executeAfterNavigation(page, newUrl); | |
| await this.pendingSetupScripts; | |
| this.pendingSetupScripts = null; | |
| const setupPromise = this.pageSetupManager.executeAfterNavigation(page, newUrl); | |
| this.pendingSetupScripts = setupPromise; | |
| try { | |
| await setupPromise; | |
| } finally { | |
| if (this.pendingSetupScripts === setupPromise) { | |
| this.pendingSetupScripts = null; | |
| } | |
| } |
Steps of Reproduction ✅
1. Start the explorer by calling Explorer.start() so listeners are installed (see
src/explorer.ts around the start() method where it calls await
this.pageSetupManager.loadSetupScripts(); and this.listenToStateChanged()). This
initializes Playwright and registers the framenavigated handler in listenToStateChanged.
2. Trigger two navigations quickly (e.g. call explorer.visit(urlA) then immediately
explorer.visit(urlB) from the same process). visit() performs navigation (src/explorer.ts
visit() function, the I.amOnPage call) which causes Playwright to emit 'framenavigated'.
3. Each framenavigated event enters the handler defined in listenToStateChanged
(src/explorer.ts at the framenavigated listener, lines shown at 328-337). The handler runs
the snippet that assigns this.pendingSetupScripts =
this.pageSetupManager.executeAfterNavigation(...); then awaits it (lines 335-337).
4. Because the code stores the promise in the shared field and an immediately subsequent
framenavigated overwrites it, the first handler's await may end up awaiting the later
promise and the finally clearing (this.pendingSetupScripts = null) may clear the field
incorrectly. Result: one setup run is left untracked or awaited incorrectly, causing setup
scripts (login/cookie dismissal) to be skipped or race between runs.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/explorer.ts
**Line:** 335:337
**Comment:**
*Race Condition: Using the shared `pendingSetupScripts` field directly for both storing and awaiting the promise in the `framenavigated` handler creates a race condition when multiple navigations happen quickly: later events overwrite the field before earlier awaits complete, so earlier handlers may end up awaiting the wrong promise and one or more setup runs can be skipped or left untracked. Capturing the promise in a local variable and only clearing the field if it still matches that promise also ensures the field is reset correctly even if the setup throws.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,100 @@ | ||||||||
| import { existsSync } from 'node:fs'; | ||||||||
| import { resolve } from 'node:path'; | ||||||||
| import type { ExplorbotConfig } from './config.js'; | ||||||||
| import type Explorer from './explorer.js'; | ||||||||
| import type { KnowledgeTracker } from './knowledge-tracker.js'; | ||||||||
| import type { StateManager } from './state-manager.js'; | ||||||||
| import { createDebug, tag } from './utils/logger.js'; | ||||||||
|
|
||||||||
| const debugLog = createDebug('explorbot:page-setup'); | ||||||||
|
|
||||||||
| export interface SetupScriptContext { | ||||||||
| page: any; | ||||||||
| context: any; | ||||||||
| explorer: Explorer; | ||||||||
| stateManager: StateManager; | ||||||||
| knowledgeTracker: KnowledgeTracker; | ||||||||
| config: ExplorbotConfig; | ||||||||
| log: (...args: any[]) => void; | ||||||||
| } | ||||||||
|
|
||||||||
| interface SetupScriptModule { | ||||||||
| setup: (context: SetupScriptContext) => Promise<void>; | ||||||||
| } | ||||||||
|
|
||||||||
| export class PageSetupManager { | ||||||||
| private scripts: SetupScriptModule[] = []; | ||||||||
| private explorer: Explorer; | ||||||||
| private config: ExplorbotConfig; | ||||||||
| private isLoaded = false; | ||||||||
|
|
||||||||
| constructor(explorer: Explorer, config: ExplorbotConfig) { | ||||||||
| this.explorer = explorer; | ||||||||
| this.config = config; | ||||||||
| } | ||||||||
|
|
||||||||
| async loadSetupScripts(): Promise<void> { | ||||||||
| if (this.isLoaded) return; | ||||||||
|
|
||||||||
| const scriptPaths = this.config.playwright?.setupScripts || []; | ||||||||
| if (scriptPaths.length === 0) { | ||||||||
| debugLog('No setup scripts configured'); | ||||||||
| this.isLoaded = true; | ||||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| for (const scriptPath of scriptPaths) { | ||||||||
| try { | ||||||||
| const fullPath = resolve(scriptPath); | ||||||||
|
|
||||||||
| if (!existsSync(fullPath)) { | ||||||||
| tag('warning').log(`Setup script not found: ${scriptPath}`); | ||||||||
| continue; | ||||||||
| } | ||||||||
|
|
||||||||
| const module = await import(fullPath); | ||||||||
|
|
||||||||
| if (typeof module.setup !== 'function') { | ||||||||
| tag('warning').log(`Setup script ${scriptPath} does not export a setup() function`); | ||||||||
| continue; | ||||||||
| } | ||||||||
|
|
||||||||
| this.scripts.push(module); | ||||||||
| debugLog(`Loaded setup script: ${scriptPath}`); | ||||||||
| tag('substep').log(`Loaded setup script: ${scriptPath}`); | ||||||||
| } catch (error) { | ||||||||
| tag('error').log(`Failed to load setup script ${scriptPath}: ${error}`); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| this.isLoaded = true; | ||||||||
| } | ||||||||
|
|
||||||||
| async executeAfterNavigation(page: any, url: string): Promise<void> { | ||||||||
| if (this.scripts.length === 0) return; | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The Severity Level: Major
|
||||||||
| if (this.scripts.length === 0) return; | |
| await this.loadSetupScripts(); | |
Steps of Reproduction ✅
1. Configure Playwright setup scripts in runtime config (Explorbot config passed into
Explorer). The PageSetupManager expects script paths to come from
config.playwright.setupScripts (see src/page-setup-manager.ts: lines defining
loadSetupScripts at lines 36-71 in the PR hunk).
2. Start the Explorer and navigate pages so the Playwright page emits frame navigation. In
src/explorer.ts (file excerpt in Additional Downstream Code Context, within the page
'framenavigated' handler) the code calls:
this.pendingSetupScripts = this.pageSetupManager.executeAfterNavigation(page, newUrl);
await this.pendingSetupScripts;
(this is the only invocation path that runs setup scripts after navigation).
3. Execution enters PageSetupManager.executeAfterNavigation at src/page-setup-manager.ts
lines 73-87. That function immediately returns when this.scripts.length === 0 (line 74),
but loadSetupScripts() is never invoked here and there is no other guaranteed call site in
the shown codebase that loads scripts prior to first navigation.
4. Observe behavior: despite having configured setup script paths, setup code never runs
after navigation (no cookie dismissal, no auto-login). The check at
src/page-setup-manager.ts line 74 causes silent early return because this.scripts is still
empty (loadSetupScripts was not awaited or invoked). This reproduces reliably whenever
setupScripts are configured but loadSetupScripts hasn't been called before the first frame
navigation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/page-setup-manager.ts
**Line:** 74:74
**Comment:**
*Possible Bug: The `executeAfterNavigation` method assumes that `loadSetupScripts` has already been called and completed, so if callers forget to load scripts or don't await the loader, `this.scripts` remains empty and setup scripts silently never run after navigation. Because `loadSetupScripts` is asynchronous and guarded by `isLoaded`, it is safe and more robust to call it from `executeAfterNavigation` itself so scripts are always loaded before execution.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks dependency hierarchy
action and managed by explorer, not vice versa