-
Notifications
You must be signed in to change notification settings - Fork 1.2k
add BB session URL & debug URL accessors #1281
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: 7482e95 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
Greptile Summary
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant V3
participant Browserbase
User->>V3: init() with env="BROWSERBASE"
V3->>Browserbase: createBrowserbaseSession()
Browserbase-->>V3: sessionId, ws, bb
V3->>V3: store sessionId
V3->>Browserbase: bb.sessions.debug(sessionId)
Browserbase-->>V3: debuggerUrl
V3->>V3: set browserbaseSessionUrl & browserbaseDebugUrl
V3-->>User: initialization complete
User->>V3: browserbaseSessionURL
V3-->>User: return stored URL
User->>V3: browserbaseDebugURL
V3-->>User: return stored debug URL
|
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.
2 files reviewed, 1 comment
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
packages/core/lib/v3/v3.ts
Outdated
| this.ctx.conn.onTransportClosed(this._onCdpClosed); | ||
| this.state = { | ||
| kind: "LOCAL", | ||
| // no LaunchedChrome when attaching externally; create a stub kill | ||
| chrome: { | ||
| kill: async () => {}, | ||
| } as unknown as import("chrome-launcher").LaunchedChrome, | ||
| ws: lbo.cdpUrl, | ||
| }; | ||
| this.resetBrowserbaseSessionMetadata(); | ||
| // Post-connect settings (downloads and viewport) if provided | ||
| await this._applyPostConnectLocalOptions(lbo); | ||
| return; |
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.
syntax: indentation lost here - should be indented to match the surrounding if block (12 spaces)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/lib/v3/v3.ts
Line: 681:693
Comment:
**syntax:** indentation lost here - should be indented to match the surrounding if block (12 spaces)
How can I resolve this? If you propose a fix, please make it concise.
why
currently, stagehand just logs the BB session URL and debug URL inline without storing this info on the stagehand object but we want getters for these.
If env = browserbase, we return the urls, if it's local we return an empty string currently as per the ticket. It seemed to me like returning undefined if env = local might be better DX, but i'll double check w/ sean.
what changed
Added getters and a small
resetBrowserbaseSessionMetadata()helper.test plan
Adde vitest tests that show that this works when running with
env: "BROWSERBASE"and returns an empty string whenenv: "LOCAL".