fix: handle page close events and avoid errors on closed targets #1496
Conversation
|
- Add handling for page close events to update active page when current page closes - Add error handling for CDP operations on closed targets - Remove unnecessary inline comments 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
shrey150
force-pushed
the
support/mahesh
branch
from
31914ca to
0e2e5ac
Compare
Greptile SummaryThis PR adds automatic page switching when pages close and graceful error handling for CDP operations on closed targets. The changes help prevent errors in multi-tab scenarios by detecting closed pages and switching focus to remaining open pages. Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Context as BrowserContext
participant SC as StagehandContext
participant Page as PlaywrightPage
participant Handler as Event Handlers
Note over Context,Handler: New Page Created
Context->>SC: emit "page" event
SC->>SC: handleNewPlaywrightPage (1st call)
SC->>Handler: Register close handler #1
SC->>Page: Check if closed
alt Page already closed
SC-->>Context: Return early
else Page open
SC->>SC: Create/get StagehandPage
SC->>SC: setActivePage
end
SC->>SC: attachFrameNavigatedListener
SC->>Handler: Register close handler #2
SC->>Page: Create CDP session
SC->>SC: handleNewPlaywrightPage (2nd call in finally)
SC->>Handler: Register close handler #3
Note over Context,Handler: Page Closes
Page->>Handler: emit "close" event
Handler->>SC: close handler #1 fires
SC->>SC: Get open pages, setActivePage
Handler->>SC: close handler #2 fires
SC->>SC: Unregister frameId
SC->>SC: Get open pages, setActivePage (duplicate)
Handler->>SC: close handler #3 fires
SC->>SC: Get open pages, setActivePage (duplicate)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (2)
-
lib/StagehandContext.ts, line 176-188 (link)logic: duplicate close handler registered -
handleNewPlaywrightPageis called twice per page (lines 96 and 107), registering this close handler twice. combined with the close handler inattachFrameNavigatedListener(lines 233-248), this creates 3 handlers that all attempt to switch the active page when a page closes. while functionally this may work due to the conditional check, it causes unnecessary iterations andsetActivePagecalls. -
lib/StagehandContext.ts, line 174 (link)logic: race condition - checking
isClosed()before registering the close handler doesn't prevent the page from closing between the check and the handler registration. if the page closes after line 174 but before line 176, the close handler won't fire and the active page won't be updated.the current pattern should use a try-catch around the
onceregistration or checkisClosed()again inside the handler. is this scenario (page closing between isClosed check and handler registration) a realistic concern in your testing, or is the check primarily for performance optimization?
1 file reviewed, 5 comments
| msg.includes("Target closed") || | ||
| msg.includes("Target page, context or browser has been closed") | ||
| ) { | ||
| return; | ||
| } | ||
| throw err; | ||
| } | ||
|
|
||
| pwPage.once("close", () => { | ||
| if (shPage.frameId) this.unregisterFrameId(shPage.frameId); | ||
|
|
||
| if (this.activeStagehandPage === shPage) { | ||
| const openPages = this.intContext.pages(); | ||
| if (openPages.length > 0) { | ||
| for (const p of openPages) { | ||
| const sp = this.pageMap.get(p); |
There was a problem hiding this comment.
style: duplicate close event logic - this close handler has the same active page switching logic as the one in handleNewPlaywrightPage (lines 176-188). the only difference is line 234 which unregisters the frame ID. consider consolidating into a single close handler or removing the duplicate switching logic from one of them.
| msg.includes("Target closed") || | |
| msg.includes("Target page, context or browser has been closed") | |
| ) { | |
| return; | |
| } | |
| throw err; | |
| } | |
| pwPage.once("close", () => { | |
| if (shPage.frameId) this.unregisterFrameId(shPage.frameId); | |
| if (this.activeStagehandPage === shPage) { | |
| const openPages = this.intContext.pages(); | |
| if (openPages.length > 0) { | |
| for (const p of openPages) { | |
| const sp = this.pageMap.get(p); | |
| pwPage.once("close", () => { | |
| if (shPage.frameId) this.unregisterFrameId(shPage.frameId); | |
| // Active page switching is handled in handleNewPlaywrightPage | |
| }); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/StagehandContext.ts
Line: 233:248
Comment:
**style:** duplicate close event logic - this close handler has the same active page switching logic as the one in `handleNewPlaywrightPage` (lines 176-188). the only difference is line 234 which unregisters the frame ID. consider consolidating into a single close handler or removing the duplicate switching logic from one of them.
```suggestion
pwPage.once("close", () => {
if (shPage.frameId) this.unregisterFrameId(shPage.frameId);
// Active page switching is handled in handleNewPlaywrightPage
});
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| public getActivePage(): StagehandPage | null { | ||
| return this.activeStagehandPage; | ||
| } |
There was a problem hiding this comment.
logic: this.intContext.pages() may include the closing page - when the close event fires, context.pages() might still return the page that's being closed. the loop should verify each page isn't closed before setting it as active.
| } | |
| const openPages = this.intContext.pages().filter(p => !p.isClosed()); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/StagehandContext.ts
Line: 179:179
Comment:
**logic:** `this.intContext.pages()` may include the closing page - when the close event fires, `context.pages()` might still return the page that's being closed. the loop should verify each page isn't closed before setting it as active.
```suggestion
const openPages = this.intContext.pages().filter(p => !p.isClosed());
```
How can I resolve this? If you propose a fix, please make it concise.| ) { | ||
| return; | ||
| } | ||
| throw err; |
There was a problem hiding this comment.
logic: same issue - filter out closed pages before attempting to set as active
| throw err; | |
| const openPages = this.intContext.pages().filter(p => !p.isClosed()); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/StagehandContext.ts
Line: 238:238
Comment:
**logic:** same issue - filter out closed pages before attempting to set as active
```suggestion
const openPages = this.intContext.pages().filter(p => !p.isClosed());
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="lib/StagehandContext.ts">
<violation number="1" location="lib/StagehandContext.ts:181">
P1: Same type mismatch issue: `this.intContext.pages()` returns wrapped `Page[]` objects, but `pageMap` expects `PlaywrightPage` keys. This lookup will always fail, preventing the active page switching from working.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (shPage && this.activeStagehandPage === shPage) { | ||
| const openPages = this.intContext.pages(); | ||
| for (const p of openPages) { | ||
| const sp = this.pageMap.get(p); |
There was a problem hiding this comment.
P1: Same type mismatch issue: this.intContext.pages() returns wrapped Page[] objects, but pageMap expects PlaywrightPage keys. This lookup will always fail, preventing the active page switching from working.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/StagehandContext.ts, line 181:
<comment>Same type mismatch issue: `this.intContext.pages()` returns wrapped `Page[]` objects, but `pageMap` expects `PlaywrightPage` keys. This lookup will always fail, preventing the active page switching from working.</comment>
<file context>
@@ -179,23 +171,80 @@ export class StagehandContext {
+ if (shPage && this.activeStagehandPage === shPage) {
+ const openPages = this.intContext.pages();
+ for (const p of openPages) {
+ const sp = this.pageMap.get(p);
+ if (sp && sp !== shPage) {
+ this.setActivePage(sp);
</file context>
Summary
Test plan
🤖 Generated with Claude Code
Summary by cubic
Keeps the active page in sync when tabs close and prevents CDP errors on closed targets. Improves multi-tab stability and avoids crashes when a page or context shuts down.
Bug Fixes
Refactors
Written for commit 0e2e5ac. Summary will update on new commits.