Add browse dialog command for auto-handling JS dialogs#1909
Open
derekmeegan wants to merge 3 commits intomainfrom
Open
Add browse dialog command for auto-handling JS dialogs#1909derekmeegan wants to merge 3 commits intomainfrom
browse dialog command for auto-handling JS dialogs#1909derekmeegan wants to merge 3 commits intomainfrom
Conversation
Adds dialog accept/dismiss/off modes via CDP Page.javascriptDialogOpening and Page.handleJavaScriptDialog. Handles alert, confirm, prompt, and beforeunload dialogs. Includes dialog_history command to inspect handled dialogs with type, message, and timestamp. Usage: browse dialog accept # auto-accept all dialogs browse dialog dismiss # auto-dismiss all dialogs browse dialog off # stop auto-handling browse dialog_history # show handled dialog log Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 03e5aca The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 2/5
- There is a high-confidence runtime failure risk in
packages/cli/src/index.ts:Page.handleJavaScriptDialogcan throw if the dialog closes or navigation happens first, and without atry/catchthis can break the async flow and user command execution. - The global
dialogHandlerInstalledboolean inpackages/cli/src/index.tscan prevent listener attachment for later tabs/sessions, creating a concrete regression wherebrowse dialog acceptmay stop working outside the first active context. - Given one severe (9/10) and one medium-high (6/10) user-facing behavior issue in core dialog handling, this looks risky to merge until those guards are in place.
- Pay close attention to
packages/cli/src/index.ts- dialog handling needs stronger error isolation and per-session listener lifecycle management.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/index.ts">
<violation number="1" location="packages/cli/src/index.ts:1602">
P2: The `dialogHandlerInstalled` flag is a global boolean, meaning the event listener will only ever be attached to the first active tab/session. If the user navigates to a new tab or runs `browse dialog accept` again on a different page, the listener won't be attached to the new `cdpSession`. Track installation per-session (e.g., using a `WeakSet<CDPSessionLike>`) instead.</violation>
<violation number="2" location="packages/cli/src/index.ts:1618">
P0: Wrap the CDP `Page.handleJavaScriptDialog` call in a `try/catch` block. If the dialog closes externally or the page navigates before this command resolves, the CDP call will throw an error. Because this is inside an async event listener, the unhandled promise rejection will trigger the daemon's `unhandledRejection` handler and crash the entire CLI process.</violation>
</file>
Architecture diagram
sequenceDiagram
participant User
participant CLI as Browse CLI
participant State as Internal State
participant CDP as CDP Session
participant Browser
Note over User, Browser: NEW: Dialog Handling Configuration
User->>CLI: browse dialog <accept|dismiss>
CLI->>State: CHANGED: Update dialogMode
opt NEW: First time initialization
CLI->>CDP: NEW: Subscribe to Page.javascriptDialogOpening
CDP-->>CLI: Subscription active
end
CLI-->>User: Mode updated
Note over User, Browser: NEW: Automated Dialog Interaction (Async)
Browser->>CDP: Trigger JS Dialog (Alert/Confirm/Prompt)
CDP->>CLI: Event: Page.javascriptDialogOpening
CLI->>State: NEW: Check current dialogMode
State-->>CLI: mode (accept/dismiss)
CLI->>State: NEW: Push entry to dialogHistory
alt mode == "accept"
CLI->>CDP: NEW: Page.handleJavaScriptDialog (accept: true)
else mode == "dismiss"
CLI->>CDP: NEW: Page.handleJavaScriptDialog (accept: false)
end
CDP->>Browser: Execute dialog action
Note over User, Browser: NEW: History Inspection
User->>CLI: browse dialog_history
CLI->>State: Fetch dialogHistory
State-->>CLI: [{type, message, handled, timestamp}]
CLI-->>User: Display formatted history log
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Wrap Page.handleJavaScriptDialog in try/catch to prevent unhandled rejections from crashing the daemon when dialogs close externally or navigation occurs before the response resolves. - Replace global boolean with WeakSet<object> to track listener installation per CDP session, so dialog handling works correctly across tab switches and new pages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/index.ts">
<violation number="1" location="packages/cli/src/index.ts:1613">
P2: `dialog_history` can report dialogs as handled even when CDP handling fails, because entries are pushed before `Page.handleJavaScriptDialog` succeeds and errors are silently swallowed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+1613
to
+1629
| dialogHistory.push({ | ||
| type: params.type, | ||
| message: params.message, | ||
| handled: dialogMode, | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
| try { | ||
| await cdpSession.send("Page.handleJavaScriptDialog", { | ||
| accept: dialogMode === "accept", | ||
| promptText: | ||
| dialogMode === "accept" | ||
| ? (params.defaultPrompt ?? "") | ||
| : undefined, | ||
| }); | ||
| } catch { | ||
| // Dialog may have been closed by navigation or externally | ||
| } |
Contributor
There was a problem hiding this comment.
P2: dialog_history can report dialogs as handled even when CDP handling fails, because entries are pushed before Page.handleJavaScriptDialog succeeds and errors are silently swallowed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/index.ts, line 1613:
<comment>`dialog_history` can report dialogs as handled even when CDP handling fails, because entries are pushed before `Page.handleJavaScriptDialog` succeeds and errors are silently swallowed.</comment>
<file context>
@@ -1599,35 +1599,41 @@ async function executeCommand(
+ defaultPrompt?: string;
+ }) => {
+ if (dialogMode === "off") return;
+ dialogHistory.push({
+ type: params.type,
+ message: params.message,
</file context>
Suggested change
| dialogHistory.push({ | |
| type: params.type, | |
| message: params.message, | |
| handled: dialogMode, | |
| timestamp: new Date().toISOString(), | |
| }); | |
| try { | |
| await cdpSession.send("Page.handleJavaScriptDialog", { | |
| accept: dialogMode === "accept", | |
| promptText: | |
| dialogMode === "accept" | |
| ? (params.defaultPrompt ?? "") | |
| : undefined, | |
| }); | |
| } catch { | |
| // Dialog may have been closed by navigation or externally | |
| } | |
| try { | |
| await cdpSession.send("Page.handleJavaScriptDialog", { | |
| accept: dialogMode === "accept", | |
| promptText: | |
| dialogMode === "accept" | |
| ? (params.defaultPrompt ?? "") | |
| : undefined, | |
| }); | |
| dialogHistory.push({ | |
| type: params.type, | |
| message: params.message, | |
| handled: dialogMode, | |
| timestamp: new Date().toISOString(), | |
| }); | |
| } catch { | |
| // Dialog may have been closed by navigation or externally | |
| } |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/index.ts">
<violation number="1" location="packages/cli/src/index.ts:2831">
P1: Custom agent: **Exception and error message sanitization**
New dialog subcommands print raw caught error messages directly to users instead of sanitized, typed error output.</violation>
<violation number="2" location="packages/cli/src/index.ts:2865">
P2: This change removes the top-level `browse dialog_history` command and only exposes `browse dialog history`, creating a CLI compatibility break for existing usage.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -854,6 +854,18 @@ let networkCounter = 0; | |||
| let networkSession: string | null = null; | |||
Contributor
There was a problem hiding this comment.
P1: Custom agent: Exception and error message sanitization
New dialog subcommands print raw caught error messages directly to users instead of sanitized, typed error output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/index.ts, line 2831:
<comment>New dialog subcommands print raw caught error messages directly to users instead of sanitized, typed error output.</comment>
<file context>
@@ -2815,29 +2815,54 @@ networkCmd
+ const result = await runCommand("dialog", ["accept"]);
+ output(result, opts.json ?? false);
+ } catch (e) {
+ console.error("Error:", e instanceof Error ? e.message : e);
process.exit(1);
}
</file context>
| }); | ||
|
|
||
| dialogCmd | ||
| .command("history") |
Contributor
There was a problem hiding this comment.
P2: This change removes the top-level browse dialog_history command and only exposes browse dialog history, creating a CLI compatibility break for existing usage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/index.ts, line 2865:
<comment>This change removes the top-level `browse dialog_history` command and only exposes `browse dialog history`, creating a CLI compatibility break for existing usage.</comment>
<file context>
@@ -2815,29 +2815,54 @@ networkCmd
+ });
+
+dialogCmd
+ .command("history")
.description("Show history of handled dialogs")
.action(async () => {
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
browse dialog <accept|dismiss|off>to auto-handle alert/confirm/prompt/beforeunload dialogs via CDPbrowse dialog_historyto inspect previously handled dialogs (type, message, timestamp, action taken)Page.javascriptDialogOpeningevent +Page.handleJavaScriptDialogresponseUsage
Test results
dialog accept+eval "alert('test')"dialog dismiss+eval "confirm('sure?')"false, logged as dismissdialog_history🤖 Generated with Claude Code