Conversation
Add PageSetupManager to execute setup scripts after navigation. Update .gitignore to exclude explorbot.config.js user config.
|
CodeAnt AI is reviewing your PR. |
codeant-ai
bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| 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.
Suggestion: 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. [possible bug]
Severity Level: Major ⚠️
- ❌ Action.execute may crash on misconfigured explorer.
- ❌ capturePageState() not reached after action.
- ⚠️ Before-hook setup scripts silently not awaited.
- ⚠️ Test flows using manual Action construction affected.| 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.| await action.execute(`I.waitForElement(${JSON.stringify(waitForElement)})`); | ||
| } | ||
|
|
||
| await this.pageSetupManager.executeAfterNavigation(this.playwrightHelper.page, url); |
There was a problem hiding this comment.
Suggestion: 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. [logic error]
Severity Level: Critical 🚨
- ❌ Duplicate setup runs cause double form submissions.
- ❌ Cookie-banner dismissal may run twice unexpectedly.
- ⚠️ Explorer.visit flows may produce flaky captures.| await this.pageSetupManager.executeAfterNavigation(this.playwrightHelper.page, url); | |
| await this.waitForPendingSetupScripts(); |
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.| this.pendingSetupScripts = this.pageSetupManager.executeAfterNavigation(page, newUrl); | ||
| await this.pendingSetupScripts; | ||
| this.pendingSetupScripts = null; |
There was a problem hiding this comment.
Suggestion: 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. [race condition]
Severity Level: Major ⚠️
- ❌ Page setup scripts tracking becomes unreliable.
- ❌ Auto-login or cookie dismissal may be skipped.
- ⚠️ capturePageState may capture preconditioned pages incorrectly.| 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.| } | ||
|
|
||
| async executeAfterNavigation(page: any, url: string): Promise<void> { | ||
| if (this.scripts.length === 0) return; |
There was a problem hiding this comment.
Suggestion: 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. [possible bug]
Severity Level: Major ⚠️
- ❌ Post-navigation setup scripts silently never execute.
- ⚠️ Cookie banners remain and block interactions.
- ⚠️ Auto-login/setup flows don't run before capture.
- ⚠️ Explorer framenavigated handler continues without preconditions.| 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.
|
CodeAnt AI finished reviewing your PR. |
DavertMik
left a comment
DavertMik
left a comment
There was a problem hiding this comment.
Valid purpose and good idea but implementation contradicts current architecture
I will add an alternative option for it
| this.experienceTracker = stateManager.getExperienceTracker(); | ||
| this.config = ConfigParser.getInstance().getConfig(); | ||
| this.playwrightHelper = container.helpers('Playwright'); | ||
| this.explorer = explorer; |
There was a problem hiding this comment.
this breaks dependency hierarchy
action and managed by explorer, not vice versa
2 participants
User description
Adding before hook script with Playwright API called after navigation, before capturing the page state. Allows to execute general prepare state preconditions as resolve login page redirect, removing the cookie banner, API login, etc..
Scripts can be placed in "browser-scripts" folder. Here is an example of a simple script:
CodeAnt-AI Description
Run optional Playwright setup scripts after navigation to prepare page state
What Changed
Impact
✅ Captures reflect post-login and post-consent✅ Fewer flaky captures from cookie banners or navigation redirects✅ Less manual setup needed to reproduce state-dependent issues💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.