-
Notifications
You must be signed in to change notification settings - Fork 966
default use API true when using browserbase #801
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
Conversation
🦋 Changeset detectedLatest commit: c448ed3 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 |
evals/test-results/.last-run.json
Outdated
@@ -0,0 +1,4 @@ | |||
{ |
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.
do we need?
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.
console.log(session); | ||
console.log(session.connectUrl); |
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.
use expect here
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.
lib/api.ts
Outdated
const region = browserbaseSessionCreateParams?.region; | ||
if (region && region !== "us-west-2") { | ||
throw new StagehandAPIError( | ||
"Only us-west-2 region is currently supported for API mode. Try updating your region or set useAPI to false.", |
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 shouldn't throw error
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.
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.
@kamath wouldn't it be better to throw here? i personally would not want to spend time guessing why code is slow. the only problem i could see is if you for some reason we're unable to set it to us-west-2
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.
PR Summary
Changed default configuration to prefer Browserbase API integration by making useAPI
default to true in Stagehand constructor, with graceful fallbacks for non-API environments.
- Modified
lib/index.ts
to defaultuseAPI=true
in constructor with silent fallback to local mode - Added region-specific handling in
lib/api.ts
for non-us-west-2 deployments - Updated session ID management to only override with
browserbaseSessionID
when available - Removed
stagehand.close()
from individual session test while maintaining cleanup inafterAll()
- Added fallback API URL when
STAGEHAND_API_URL
environment variable is not set
3 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile
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 main, this PR will be updated. # Releases ## @browserbasehq/[email protected] ### Patch Changes - [#796](#796) [`12a99b3`](12a99b3) Thanks [@miguelg719](https://github.com/miguelg719)! - Added a experimental flag to enable the newest and most experimental features - [#807](#807) [`2451797`](2451797) Thanks [@seanmcguire12](https://github.com/seanmcguire12)! - include version number in StagehandDefaultError message - [#803](#803) [`1d631a5`](1d631a5) Thanks [@miguelg719](https://github.com/miguelg719)! - Enable session affinity for cache optimization - [#804](#804) [`9c398bb`](9c398bb) Thanks [@seanmcguire12](https://github.com/seanmcguire12)! - update operatorResponseSchema based on new openai spec - [#786](#786) [`c19ad7f`](c19ad7f) Thanks [@miguelg719](https://github.com/miguelg719)! - Handle reroute to account for rollout ## @browserbasehq/[email protected] ### Minor Changes - [#778](#778) [`df570b6`](df570b6) Thanks [@seanmcguire12](https://github.com/seanmcguire12)! - iframe support ### Patch Changes - [#809](#809) [`03ebebc`](03ebebc) Thanks [@seanmcguire12](https://github.com/seanmcguire12)! - log NoObjectGenerated error details - [#801](#801) [`1d4f0ab`](1d4f0ab) Thanks [@miguelg719](https://github.com/miguelg719)! - Default use API to true - [#798](#798) [`d86200b`](d86200b) Thanks [@miguelg719](https://github.com/miguelg719)! - Fix pino logging memory leak by reusing worker ## @browserbasehq/[email protected] ### Patch Changes - Updated dependencies \[[`12a99b3`](12a99b3), [`2451797`](2451797), [`1d631a5`](1d631a5), [`9c398bb`](9c398bb), [`c19ad7f`](c19ad7f)]: - @browserbasehq/[email protected] ## @browserbasehq/[email protected] ### Patch Changes - Updated dependencies \[[`12a99b3`](12a99b3), [`2451797`](2451797), [`1d631a5`](1d631a5), [`9c398bb`](9c398bb), [`c19ad7f`](c19ad7f)]: - @browserbasehq/[email protected] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
why
what changed
test plan