Dismiss alert lead to take screenshot #2219
Conversation
on classical page with alert the browser fall un timeout beacause alert block loading. This pr lead to close alert and take screen
WalkthroughScreenshotWithBody now registers a PageJavascriptDialogOpening event handler (via page.EachEvent) in a goroutine that calls PageHandleJavaScriptDialog with Accept:true and empty PromptText to auto-accept dialogs before headers, navigation, and screenshot capture. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser.ScreenshotWithBody
participant Page as page
participant Goroutine as Dialog Goroutine
Browser->>Page: start goroutine: page.EachEvent(PageJavascriptDialogOpening, handler)
Note right of Goroutine #F0F8FF: handler -> PageHandleJavaScriptDialog(Accept:true, PromptText:"")
Browser->>Page: set headers
Browser->>Page: navigate / interact
alt JavaScript dialog opens
Page->>Goroutine: emit PageJavascriptDialogOpening event
Goroutine->>Page: invoke PageHandleJavaScriptDialog(Accept:true, PromptText:"")
end
Browser->>Browser: continue timeout/wait/screenshot/html retrieval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
runner/headless.go (2)
107-114: Security/regression caution: auto-accepts all dialogs, not just alerts.Accepting confirm/prompt can trigger unintended actions. Consider restricting to alerts only, or making behavior configurable.
Example approach (conceptual):
// Pseudocode: react based on dialog type wait := page.WaitEvent(&proto.PageJavascriptDialogOpening{}) go func() { for { ev := wait().(*proto.PageJavascriptDialogOpening) accept := ev.Type == "alert" // dismiss only alerts by "OK" _ = proto.PageHandleJavaScriptDialog{Accept: accept}.Call(page) } }()Would you prefer a follow-up PR that introduces a flag (e.g., --dialog-policy=alerts-only|accept-all|dismiss-non-alerts) and appropriate logging?
110-113: Check and handle error returned by handle() (fix errcheck)handle(&proto.PageHandleJavaScriptDialog{...}) returns an error — check it to satisfy errcheck and avoid silent failures.
File: runner/headless.go lines 110–113
- handle(&proto.PageHandleJavaScriptDialog{ - Accept: true, - PromptText: "", - }) + if err := handle(&proto.PageHandleJavaScriptDialog{ + Accept: true, + PromptText: "", + }); err != nil { + // best-effort: ignore dialog handling errors (e.g., page closed) + }After patch, verify with: golangci-lint run ./...
🧹 Nitpick comments (1)
runner/headless.go (1)
107-114: Make dialog handling resilient to multiple dialogs and panics.Handle more than one dialog and guard against potential panics if the page closes mid-wait.
Apply this diff:
- go func() { - wait() - if err := handle(&proto.PageHandleJavaScriptDialog{ - Accept: true, - PromptText: "", - }); err != nil { - // best-effort: ignore dialog handling errors (e.g., page closed) to avoid crashing - } - }() + go func() { + defer func() { _ = recover() }() // defensive: rod may panic on page teardown + for { + wait() + if err := handle(&proto.PageHandleJavaScriptDialog{ + Accept: true, + PromptText: "", + }); err != nil { + break // stop on error (page closed, no target, etc.) + } + } + }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runner/headless.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint Test
runner/headless.go
[failure] 110-110:
Error return value is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: release-test
- GitHub Check: Analyze (go)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Functional Test (windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
runner/headless.go (1)
107-114: Safer handling: dismiss beforeunload, keep accepting others; don’t swallow errors.Accepting all dialog types can unintentionally confirm beforeunload navigations. Prefer dismissing beforeunload while still accepting alert/confirm/prompt; also surface errors (even as debug) rather than assigning to _. CDP explicitly distinguishes dialog types and semantics for Page.handleJavaScriptDialog. (chromedevtools.github.io)
Apply within this hunk:
-go page.EachEvent(func(e *proto.PageJavascriptDialogOpening) { - _ = proto.PageHandleJavaScriptDialog{ - Accept: true, - PromptText: "", - }.Call(page) -})() +go page.EachEvent(func(e *proto.PageJavascriptDialogOpening) { + // Accept alerts/confirm/prompts to unblock; dismiss beforeunload to avoid unintended navigations. + h := proto.PageHandleJavaScriptDialog{Accept: true} + switch string(e.Type) { // values: "alert" | "confirm" | "prompt" | "beforeunload" + case "beforeunload": + h.Accept = false + case "prompt": + h.PromptText = "" + } + if err := h.Call(page); err != nil { + // TODO: optional debug log; keep non-fatal to avoid impacting screenshot flow + } +})()
🧹 Nitpick comments (1)
runner/headless.go (1)
107-114: Optional: add minimal observability and guardrails.
- Log dialog type/message at verbose level for auditability and repros.
- Consider a soft cap (e.g., <= 50 dialogs/page) to avoid pathological pages firing dialog storms.
These are nice-to-haves; the current fix is functional. (github.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
runner/headless.go(1 hunks)runner/runner.go(0 hunks)
💤 Files with no reviewable changes (1)
- runner/runner.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Functional Test (windows-latest)
🔇 Additional comments (1)
runner/headless.go (1)
107-114: LGTM: event-based dialog handling prevents hangs before navigation.Registering a Page.javascriptDialogOpening listener via page.EachEvent and accepting dialogs asynchronously is the right pattern here and should eliminate alert-induced timeouts during load. This mirrors Rod’s documented approach and prior guidance to handle dialogs via events. (github.com)
|
@coderabbitai resolve |
🧩 Analysis chain@Mzack9999 I'll resolve the review comments and create an issue for this PR. 🏁 Script executed: Length of output: 53 @Mzack9999 Done! I've resolved all review comments and created issue #2267 for implementing the alert dismissal functionality: #2267 The issue includes the background context, proposed solution details, and references to this PR for tracking purposes. ✅ Actions performedComments resolved. |
2 participants
on classical page with alert the browser fall un timeout beacause alert block loading. This pr lead to close alert and take screen
Summary by CodeRabbit