-
Notifications
You must be signed in to change notification settings - Fork 660
Add session metadata for model name if env: BROWSERBASE
#765
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
base: main
Are you sure you want to change the base?
Add session metadata for model name if env: BROWSERBASE
#765
Conversation
🦋 Changeset detectedLatest commit: 3e9579c 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 |
BROWSERBASE
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
Added session metadata tracking for model names and custom client usage in Browserbase sessions, with corresponding test coverage and configuration updates.
- Added
modelName
andusingCustomClient
metadata inlib/index.ts
for Browserbase session creation - Added test
custom_client_metadata.test.ts
to verify metadata with custom LLM clients - Updated
sessions.test.ts
to verify standard model metadata tracking - Changed default model from
gpt-4o
togoogle/gemini-2.0-flash
instagehand.config.ts
- Added development tooling with
example-dev.ts
and corresponding npm scripts
8 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
modelName: undefined, | ||
llmClient: new AISdkClient({ | ||
model: google("gemini-2.0-flash"), | ||
}), |
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.
logic: Setting modelName to undefined while providing an llmClient with a specific model (gemini-2.0-flash) seems inconsistent. Consider setting modelName to match the provided model or document why this is intentionally different.
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 is intentional to mock when users don't provide a modelName
expect(session.userMetadata.stagehand).toBe("true"); | ||
expect(session.userMetadata.modelName).toBe("NO_MODEL_DEFINED"); | ||
expect(session.userMetadata.usingCustomClient).toBe("true"); |
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.
style: Values in userMetadata are stored as strings ('true', 'NO_MODEL_DEFINED'). Consider using a type-safe enum or constants for these values to prevent typos and ensure consistency.
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.
k
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
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
(updates since last review)
Reverted model configuration from Gemini back to GPT-4o and cleaned up development files.
- Changed model configuration in
stagehand.config.ts
fromgoogle/gemini-2.0-flash
back togpt-4o
- Updated
sessions.test.ts
to verify GPT-4o metadata instead of Gemini - Removed development files
example-dev.ts
and related npm scripts - Fixed
_usingCustomClient
logic inlib/index.ts
to correctly identify custom client usage
6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -739,6 +744,8 @@ export class Stagehand { | |||
this.env, | |||
this.headless, | |||
this.logger, | |||
this._usingCustomClient ? "NO_MODEL_DEFINED" : this.modelName, |
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.
style: Consider using a constant for 'NO_MODEL_DEFINED' to avoid magic strings
why
Be able to show users what model config they used in the Browserbase dashboard
what changed
Just added session create metadata to send
modelName
to the Browserbase API similar tostagehand: true
on Browserbase session createtest plan
Added new evals