Skip to content

Conversation

@seanmcguire12
Copy link
Member

@seanmcguire12 seanmcguire12 commented Dec 8, 2025

why

what changed

test plan


Summary by cubic

Moves Stagehand DOM helper injection from per-page evaluate calls to a single Playwright context init script. Removes internal page.evaluate usage and drops unused scroll/XPath helpers on window.

  • Refactors

    • Inject helper script once per context via context.addInitScript, guarded by window.__stagehandInjected.
    • Removed StagehandPage.ensureStagehandScript and its call sites.
    • Deleted DOM helpers and related modules: utils.ts, xpathUtils.ts, elementCheckUtils.ts; removed associated globals in process.ts and global.d.ts.
    • Simplified dom/index to a side-effect import.
  • Migration

    • window.getScrollableElementXpaths, window.getNodeFromXpath, and window.waitForElementScrollEnd are removed. Update any references.
    • Do not delete Stagehand globals at runtime; they won’t be re-injected.

Written for commit d4a95f3. Summary will update automatically on new commits.

@changeset-bot
Copy link

changeset-bot bot commented Dec 8, 2025

🦋 Changeset detected

Latest commit: d4a95f3

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@seanmcguire12
Copy link
Member Author

@greptileai

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 9, 2025

Greptile Overview

Greptile Summary

Refactors DOM helper script injection from per-page evaluate() calls to a single context-level init script, reducing overhead and improving reliability. The PR removes internal page.evaluate() usage by deleting ensureStagehandScript() and unused scroll/xpath helper functions that were previously migrated to CDP in #1362.

Key Changes:

  • Injects helper script once per context via context.addInitScript() with WeakSet tracking to prevent duplicate context-level injections
  • Removes StagehandPage.ensureStagehandScript() and all call sites
  • Deletes unused DOM utilities: getScrollableElementXpaths(), getNodeFromXpath(), waitForElementScrollEnd() and supporting modules (utils.ts, xpathUtils.ts, elementCheckUtils.ts)
  • Updates test to remove re-injection behavior test (no longer applicable with context-level injection)

Notes:

  • The duplicate injection at lib/index.ts:831 was already flagged in a previous review comment and appears to be a known issue

Confidence Score: 4/5

  • Safe to merge with minor optimization opportunity - duplicate injection is guarded but adds unnecessary overhead
  • The refactoring is well-executed with proper guard clauses preventing runtime issues. All deleted code has no remaining references. The duplicate injection at lib/index.ts:831 is already tracked in previous review threads and protected by the window.__stagehandInjected guard, so it won't cause functional issues, just minor inefficiency. The WeakSet tracking in StagehandContext is a solid pattern
  • No files require special attention - the duplicate injection issue at lib/index.ts:831 is already documented in previous review threads

Important Files Changed

File Analysis

Filename Score Overview
lib/StagehandContext.ts 4/5 Adds init script injection to context using WeakSet tracking. Clean implementation but creates duplicate injection with lib/index.ts:831
lib/StagehandPage.ts 5/5 Removes ensureStagehandScript() method and its call sites. Clean removal with no remaining dependencies
lib/dom/process.ts 5/5 Removes unused scroll and xpath helper functions. Correctly removes window global assignments. Only retains shadow root interception logic
evals/deterministic/tests/page/addInitScript.test.ts 5/5 Removes test for re-injection behavior. Appropriate since scripts are now injected once via context init script

Sequence Diagram

sequenceDiagram
    participant Stagehand as Stagehand.init()
    participant Context as StagehandContext.init()
    participant Playwright as Playwright Context
    participant Browser as Browser Window
    
    Stagehand->>Context: StagehandContext.init(context, this)
    Context->>Context: Check if context in WeakSet
    alt Not in WeakSet
        Context->>Playwright: context.addInitScript(stagehandInitScript)
        Context->>Context: Add context to WeakSet
    else Already in WeakSet
        Context->>Context: Skip injection
    end
    Context-->>Stagehand: Return StagehandContext instance
    
    Note over Stagehand,Browser: ⚠️ Duplicate injection occurs here
    Stagehand->>Playwright: context.addInitScript(guardedScript)
    
    Stagehand->>Browser: Create/navigate to page
    Browser->>Browser: Execute init scripts
    Browser->>Browser: Check window.__stagehandInjected
    alt Not injected
        Browser->>Browser: Set __stagehandInjected = true
        Browser->>Browser: Execute shadow root interception
    else Already injected
        Browser->>Browser: Skip execution (guarded)
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +94 to +96
if (!StagehandContext.contextsWithInitScript.has(context)) {
await context.addInitScript({ content: stagehandInitScript });
StagehandContext.contextsWithInitScript.add(context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: duplicate init script injection - lib/index.ts:831 already calls context.addInitScript() with the same script content after StagehandContext.init() returns

Suggested change
if (!StagehandContext.contextsWithInitScript.has(context)) {
await context.addInitScript({ content: stagehandInitScript });
StagehandContext.contextsWithInitScript.add(context);
// Note: init script is added in lib/index.ts after context creation
// if (!StagehandContext.contextsWithInitScript.has(context)) {
// await context.addInitScript({ content: stagehandInitScript });
// StagehandContext.contextsWithInitScript.add(context);
// }
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/StagehandContext.ts
Line: 94:96

Comment:
**logic:** duplicate init script injection - `lib/index.ts:831` already calls `context.addInitScript()` with the same script content after `StagehandContext.init()` returns

```suggestion
    // Note: init script is added in lib/index.ts after context creation
    // if (!StagehandContext.contextsWithInitScript.has(context)) {
    //   await context.addInitScript({ content: stagehandInitScript });
    //   StagehandContext.contextsWithInitScript.add(context);
    // }
```

How can I resolve this? If you propose a fix, please make it concise.

@seanmcguire12 seanmcguire12 force-pushed the rm-internal-page-evaluate-usage branch from dafa667 to 7db3209 Compare December 9, 2025 04:36
@seanmcguire12 seanmcguire12 marked this pull request as ready for review December 9, 2025 15:55
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 9 files

@seanmcguire12 seanmcguire12 merged commit dbffb75 into v2 Dec 9, 2025
5 checks passed
@github-actions github-actions bot mentioned this pull request Dec 9, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

miguelg719 pushed a commit that referenced this pull request Dec 9, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to v2, this PR will
be updated.


# Releases
## @browserbasehq/[email protected]

### Patch Changes

- [#1389](#1389)
[`575226f`](575226f)
Thanks [@seanmcguire12](https://github.com/seanmcguire12)! - Retry
DOM.getDocument on max depth exceeded with exponential backoff

- [#1386](#1386)
[`dbffb75`](dbffb75)
Thanks [@seanmcguire12](https://github.com/seanmcguire12)! - Remove
deprecated internal usage of page.evaluate

## @browserbasehq/[email protected]

### Patch Changes

- Updated dependencies
\[[`575226f`](575226f),
[`dbffb75`](dbffb75)]:
    -   @browserbasehq/[email protected]

## @browserbasehq/[email protected]

### Patch Changes

- Updated dependencies
\[[`575226f`](575226f),
[`dbffb75`](dbffb75)]:
    -   @browserbasehq/[email protected]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants