Conversation
Contributor
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 24249734240(execution 24249734240 / attempt 1) |
Contributor
WalkthroughAdds end-to-end in-page JavaScript dialog support: main-process prompt queue and IPC, preload dialog patching and Flow API, renderer ActivePrompts context and WebPrompts UI, shared prompt types/utilities, UI primitives, a view-layer stacking change, and a small devDependency bump. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer Process
participant Preload as Preload Context
participant IPCMain as IPC Main
participant Queue as Prompt Queue (Main)
participant RendererUI as Renderer UI
Renderer->>Preload: call window.prompt/confirm/alert
Preload->>IPCMain: ipcRenderer.sendSync("prompts:*", ...)
IPCMain->>Queue: queuePrompt(tabId, promptState)
Queue->>IPCMain: activePromptsChanged() [coalesced]
IPCMain->>Preload: emit "prompts:on-active-prompts-changed"
Preload->>Renderer: deliver active prompts update
Renderer->>RendererUI: ActivePromptsProvider updates -> render WebPrompts
RendererUI->>Preload: flow.prompts.confirmPrompt(promptId, result, suppress)
Preload->>IPCMain: ipcRenderer.send("prompts:confirm-prompt", promptId, result, suppress)
IPCMain->>Queue: promptCompleted(promptId, result, suppress)
Queue->>IPCMain: activePromptsChanged() -> process queue
IPCMain->>Preload: respond to blocked sync sender (original sendSync) with result
Preload->>Renderer: prompt/confirm/alert returns to caller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR adds native JavaScript dialog support (
Confidence Score: 4/5Safe to merge with one P1 suppression race condition worth fixing before shipping the suppress feature broadly. The core dialog flow (queue, display, dismiss, IPC roundtrip) is architecturally sound and the async sendSync pattern works correctly given the multi-process browser design. The P1 suppress race means the "prevent additional dialogs" checkbox can fail for the immediately-next queued dialog in multi-iframe scenarios — a real behavioral defect but limited in scope. The P2 suppressedKeys leak is a housekeeping concern. No security or data-loss risks were found. src/main/modules/prompts.ts (suppressedKeys cleanup) and src/renderer/src/components/browser-ui/web-prompts.tsx (suppress/confirm ordering) Important Files Changed
Sequence DiagramsequenceDiagram
participant TW as Tab Renderer
participant Pre as Preload
participant Main as Main Process
participant UI as Browser UI Renderer
TW->>Pre: window.confirm("Delete?")
Pre->>Main: sendSync("prompts:confirm", msg)
Note over TW,Pre: Tab renderer blocked
Main->>Main: queuePrompt(state)
Main->>Main: processPromptQueue() → activePrompts.push()
Main->>UI: on-active-prompts-changed([prompt])
UI->>UI: Show JavaScriptDialogCard
UI-->>Main: prompts:confirm-prompt(id, true)
Main->>Main: promptCompleted(id, result)
Main->>Main: event.returnValue = true
Main->>Main: processPromptQueue() next queued
Note over TW,Pre: Renderer unblocked
Pre-->>TW: returns true
opt suppress checked
UI-->>Main: prompts:suppress-prompt(key)
Main->>Main: suppressedKeys.add(key)
Note over Main: Too late if next prompt already activated
end
Reviews (2): Last reviewed commit: "refactor: move suppression to main proce..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/browser/prompts/page.ts`:
- Around line 21-25: The code currently sets event.returnValue = null when
tabsController.getTabByWebContents(webContents) or webFrame is missing; instead
return the prompt-specific fallback (so window.confirm -> false, window.alert ->
undefined, window.prompt -> null/empty as the platform contract expects). Update
the branch in tryPatchPrompts so it uses the existing prompt-type identifier
(e.g., the local promptType / promptName variable used by tryPatchPrompts) and
returns the corresponding fallback from a promptFallbacks map (or implement a
small getPromptFallback(promptType) helper) rather than hard-coding null; keep
references to tryPatchPrompts, tabsController.getTabByWebContents, and
event.returnValue when making the change.
- Around line 12-38: The current async handler processPromptRequest is written
for a synchronous ipcMain.on + event.returnValue pattern but awaits async work;
switch to using ipcMain.handle and an invoke-style handler: change the signature
to accept Electron.IpcMainInvokeEvent (or same params) and return the final
value (result.result or failedValue) instead of setting event.returnValue, then
update all IPC callers to use ipcRenderer.invoke(...) instead of
ipcRenderer.sendSync(); keep the existing logic (tabsController lookup,
queuePrompt(generatePromptState(...)), awaiting promise) but ensure the handler
returns null on missing tab/frame and returns the awaited value for invoke.
In `@src/main/modules/prompts.ts`:
- Around line 70-94: promptCompleted currently forwards IPC payloads directly;
validate and sanitize the payload per activePrompt.type before calling
activePrompt.resolver. Inside promptCompleted (switch on activePrompt.type),
enforce: for "prompt" allow only string or null (if typeof result !== "string"
&& result !== null then treat as null or return success:false), for "confirm"
accept only boolean (if typeof result !== "boolean" coerce to false or return
success:false), and for "alert" ignore any result and always return undefined
for result; then call activePrompt.resolver with a consistent { success: true,
result } when the value matches the expected shape, otherwise resolve with {
success: false, result: undefined } to avoid violating the web API contracts.
In `@src/renderer/src/components/browser-ui/web-prompts.tsx`:
- Around line 191-220: The TabWebPrompt subtree retains stale per-prompt state;
ensure it remounts when a new prompt is shown by adding a key tied to the prompt
identity (use prompt.id) on the top-level element that wraps the prompt UI
(e.g., the PortalComponent or the immediate wrapper around JavaScriptDialogCard
inside TabWebPrompt and the analogous place noted at lines 247-248), so each
prompt gets a fresh lifecycle and mount-only effects (useMount, suppression
auto-cancel) run for the new prompt.
- Around line 24-35: The code is collapsing different prompt origins and using
the top-level tab.url for scope; update getOriginFromURL and the ActivePrompt
plumbing to preserve the full origin (scheme + hostname + optional port) for
suppression/attribution rather than stripping to hostname or using tab.url:
change getOriginFromURL to return urlObject.origin for http/https as well (avoid
returning hostname-only), ensure wherever ActivePrompt instances are created you
pass the prompt's actual origin (the iframe or prompt source URL) through the
ActivePrompt constructor/props, and switch suppression and keying logic to use
that exact origin string; keep any hostname-only display formatting limited to
UI title rendering only (do not use it for keys or suppression checks).
- Around line 104-125: The keyboard listeners in the useEffect blocks attach
regardless of visibility, so add a guard that returns early if the prompt is not
visible: inside the effect that reads cardRef.current (the effect that registers
handleKeyDown and calls confirm()/cancel()), check `!isVisible` (or the
prop/state that indicates visibility) and return before computing
card.ownerDocument or adding listeners; do the same change in the second
useEffect that registers a similar keydown handler (the other block around
handleKeyDown that calls confirm/cancel) so listeners are only attached when the
prompt is visible.
In `@src/renderer/src/components/providers/active-prompts-provider.tsx`:
- Around line 25-38: The bootstrap fetch can clobber a newer update; fix by
subscribing first then fetching and guarding the fetch result with a sequence
token so late responses don't overwrite newer subscription updates: inside the
useEffect that currently references fetchActivePrompts, remove the external
fetchActivePrompts/useCallback, call flow.prompts.onActivePromptsChanged(...)
first (keep setActivePrompts in that callback), maintain a local let seq = 0 and
increment it inside the subscription callback (or set a lastUpdate token), then
call flow.prompts.getActivePrompts() and only call setActivePrompts(prompts) if
the token captured when the fetch started still equals the current token (i.e.,
no intervening subscription update); also remove useCallback from the React
import and from the code to avoid unnecessary dependency handling.
In `@src/renderer/src/components/ui/field.tsx`:
- Around line 184-193: The dedupe logic for errors currently builds uniqueErrors
from errors.map(error => error?.message) without filtering out undefined/empty
messages, which can force list rendering; update the flow in field.tsx so you
first derive a filteredMessages array from errors by mapping to error?.message
and filtering to only non-empty strings (e.g., typeof msg === 'string' &&
msg.trim() !== ''), then build uniqueErrors from that filteredMessages (e.g.,
using new Map or Set), use uniqueErrors to decide single vs list rendering, and
use the same filtered+deduped array when mapping to <li> elements instead of the
original uniqueErrors that may contain undefined.
In `@src/shared/flow/interfaces/browser/prompts.ts`:
- Around line 7-18: The docstrings for getActivePrompts and
onActivePromptsChanged incorrectly mention “conditional passkey requests”;
update both comments to describe that these methods deal with active JavaScript
dialog prompts (e.g., alert/confirm/prompt) and return/emit an array of
ActivePrompt objects representing those dialogs. Modify the summary and
`@returns/`@param text to reference "active JavaScript dialogs" or "dialog
prompts" and ensure wording on onActivePromptsChanged clarifies the callback
receives the full updated list of ActivePrompt[] when dialogs are added,
updated, or removed.
In `@src/shared/layers.ts`:
- Around line 24-30: The layer enum documentation and values are inconsistent:
OVERLAY_UNDER (30) and OVERLAY (31) break the file-level rule that layer values
are spaced by 10; update either the numeric values or the doc comment to restore
the spacing convention—e.g., change OVERLAY back to 40 (or move OVERLAY_UNDER to
30 and OVERLAY to 40) and update the surrounding comment that states "layer
values are spaced by 10" so it matches the actual numeric scheme; ensure you
edit the symbols OVERLAY_UNDER and OVERLAY (and the file-level spacing rule
comment) so future allocations remain predictable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb1b486c-9d6b-436b-abbe-a096984e38fa
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
package.jsonsrc/main/ipc/browser/prompts/browser.tssrc/main/ipc/browser/prompts/index.tssrc/main/ipc/browser/prompts/page.tssrc/main/ipc/index.tssrc/main/modules/prompts.tssrc/preload/index.tssrc/preload/prompts.tssrc/renderer/src/components/browser-ui/main.tsxsrc/renderer/src/components/browser-ui/web-prompts.tsxsrc/renderer/src/components/providers/active-prompts-provider.tsxsrc/renderer/src/components/ui/checkbox.tsxsrc/renderer/src/components/ui/field.tsxsrc/shared/flow/flow.tssrc/shared/flow/interfaces/browser/prompts.tssrc/shared/layers.tssrc/shared/types/prompts.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/browser/prompts/browser.ts`:
- Around line 15-18: The scheduled immediate handler for
activePromptsChangedImmediate can be left uncleared if
sendMessageToListeners(...) throws; wrap the
sendMessageToListeners("prompts:on-active-prompts-changed",
getActivePromptsForRenderer()) call inside a try/finally in the
activePromptsChangedImmediate callback so cleanupActivePromptsImmediate() always
runs and activePromptsChangedImmediate is reset; ensure the finally calls
cleanupActivePromptsImmediate() (or clears the immediate handle) so subsequent
activePromptsChanged() calls are not rendered into no-ops.
- Around line 25-27: The IPC handler for "prompts:confirm-prompt" forwards
arbitrary result data to promptCompleted, risking non-boolean values for confirm
dialogs; update the ipcMain.on("prompts:confirm-prompt", ...) handler to
validate the incoming result is a boolean (or coerce safely) before calling
promptCompleted(promptId, result), and if validation fails log an error and
either reject/ignore the payload or call promptCompleted with a safe default
(false); reference the "prompts:confirm-prompt" listener and the
promptCompleted(promptId, result) function when applying the fix.
In `@src/preload/prompts.ts`:
- Around line 27-80: The patch unconditionally replaces
window.prompt/confirm/alert via tryPatchPrompts which causes non-tab renderers
(popups/settings/internal windows) to get null/falsey results; update
tryPatchPrompts to detect/guard against non-tab contexts before exposing/using
electronPrompts: when creating electronPromptsContainer and before calling
contextBridge.exposeInMainWorld / executeInMainWorld, call into the same
tab-checking IPC (or reuse the logic from src/main/ipc/browser/prompts/page.ts)
to determine if the sender maps to a tab/frame, and only expose/replace
window.prompt/confirm/alert in those tab-backed renderers; for non-tab contexts
preserve the original window.prompt/confirm/alert by not overwriting them or by
adding an explicit fallback that calls the native originals instead (refer to
electronPromptsContainer, mainWorldScript, tryPatchPrompts, and the
contextBridge calls to locate where to gate the patch).
In `@src/shared/flow/interfaces/browser/prompts.ts`:
- Around line 20-25: Rename the public API confirmPrompt to completePrompt and
tighten the result type to a discriminated/generic payload (e.g., result: string
| boolean | null | undefined or a discriminated union keyed by promptType) so
callers and IPC handlers cannot pass arbitrary any; update the interface
signature confirmPrompt -> completePrompt in
src/shared/flow/interfaces/browser/prompts.ts and then refactor all usages
(search for confirmPrompt, prompts:confirm-prompt, and promptCompleted) to call
completePrompt or accept the new discriminated payload shape; ensure IPC channel
handlers that dispatch promptCompleted or handle "prompts:confirm-prompt" are
updated to validate/deserialize the new union/discriminated type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcf43fc1-6869-42c3-8a91-8b7ed8b90d9c
📒 Files selected for processing (4)
src/main/ipc/browser/prompts/browser.tssrc/preload/index.tssrc/preload/prompts.tssrc/shared/flow/interfaces/browser/prompts.ts
Member
Author
|
@greptile review |
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/shared/utility.ts (1)
3-8:⚠️ Potential issue | 🟠 MajorKeep suppression keyed by the full origin.
Line 6 drops scheme and port for
http/https. Sincesrc/main/ipc/browser/prompts/page.tsbuildssuppressionKeyfrom this helper,http://example.com,https://example.com, andhttps://example.com:8443all collapse into the same bucket, so suppressing one dialog can suppress another origin unintentionally. ReturnurlObject.originfor scoping, and use a separate display helper if the UI title should stay hostname-only.Proposed fix
export function getOriginFromURL(url: string): string { try { const urlObject = new URL(url); - const protocol = urlObject.protocol.toLowerCase(); - if (protocol === "http:" || protocol === "https:") { - return urlObject.hostname; - } return urlObject.origin; } catch { return url; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/utility.ts` around lines 3 - 8, The helper in src/shared/utility.ts that computes the suppression key currently returns urlObject.hostname for http/https which collapses scheme/port; change it to always return urlObject.origin so suppression is keyed by full origin (scheme + host + port). Update the function that builds suppressionKey (referenced where suppressionKey is used in src/main/ipc/browser/prompts/page.ts) to consume this origin value; if the UI needs a hostname-only display, add/use a separate display helper (e.g., getDisplayHost or similar) rather than changing the suppression key logic.src/shared/flow/interfaces/browser/prompts.ts (1)
20-25:⚠️ Potential issue | 🟠 MajorDon’t expose dialog completion as
any.
result: anyerases the only compile-time contract for alert/confirm/prompt, and the current preload/main path forwards that payload straight intopromptCompleted()(src/preload/index.ts:660-666,src/main/ipc/browser/prompts/browser.ts:25-28). A bad caller can makewindow.confirm()resolve to non-booleans orwindow.prompt()resolve to objects. Tighten this tostring | boolean | null | undefined(or a discriminated payload) and reject mismatches at the IPC boundary.Proposed interface tightening
-/* eslint-disable `@typescript-eslint/no-explicit-any` */ - import { IPCListener } from "~/flow/types"; import { ActivePrompt } from "~/types/prompts"; @@ - confirmPrompt: (promptId: string, result: any) => void; + confirmPrompt: (promptId: string, result: string | boolean | null | undefined) => void;#!/bin/bash set -euo pipefail # Verify every call path that currently accepts/forwards the prompt completion payload. rg -n -C2 '\bconfirmPrompt\b|\bprompts:confirm-prompt\b|\bpromptCompleted\s*\(' --glob '*.ts' --glob '*.tsx'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/flow/interfaces/browser/prompts.ts` around lines 20 - 25, The confirmPrompt signature currently uses result: any which allows invalid prompt completions; change the type in the interface confirmPrompt (in src/shared/flow/interfaces/browser/prompts.ts) from any to the tightened union string | boolean | null | undefined (or a discriminated union) and then add validation at the IPC boundary where promptCompleted is forwarded (preload/index.ts around promptCompleted usage and main ipc handler in src/main/ipc/browser/prompts/browser.ts) to reject/throw or return an error for values that don’t match those types; ensure callers that forward window.confirm/window.prompt results are updated to coerce/guard to string|boolean|null|undefined before calling confirmPrompt and update any tests/types that relied on any.src/main/modules/prompts.ts (1)
77-101:⚠️ Potential issue | 🟠 MajorSanitize completion payloads by dialog type before resolving.
promptCompleted()still forwards the raw IPC payload back to the page. A malformed browser-UI call can therefore makeconfirm()resolve to a non-boolean,prompt()to a non-string/non-null, oralert()to arbitrary data.🛡️ Proposed fix
export function promptCompleted(promptId: string, result: any) { const activePrompt = removePromptById(activePrompts, promptId); if (!activePrompt) return false; activePromptsChanged(); switch (activePrompt.type) { case "prompt": activePrompt.resolver({ success: true, - result + result: typeof result === "string" || result === null ? result : null }); break; case "confirm": activePrompt.resolver({ success: true, - result + result: result === true }); break; case "alert": activePrompt.resolver({ success: true, - result + result: undefined }); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/modules/prompts.ts` around lines 77 - 101, promptCompleted currently forwards raw IPC payloads; validate/sanitize per dialog type before calling activePrompt.resolver: in promptCompleted (use activePrompt.type) ensure "confirm" always resolves with a boolean (coerce via Boolean(result) or default to false if value is not boolean-like), ensure "prompt" resolves with either a string or null (if result is null keep null, otherwise cast non-strings to String(result) or default to null), and ensure "alert" does not expose arbitrary data (pass undefined or omit result). Replace the current direct pass-through in each case with these validated/coerced values so malformed UI payloads cannot change the expected types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/modules/prompts.ts`:
- Around line 77-101: promptCompleted currently forwards raw IPC payloads;
validate/sanitize per dialog type before calling activePrompt.resolver: in
promptCompleted (use activePrompt.type) ensure "confirm" always resolves with a
boolean (coerce via Boolean(result) or default to false if value is not
boolean-like), ensure "prompt" resolves with either a string or null (if result
is null keep null, otherwise cast non-strings to String(result) or default to
null), and ensure "alert" does not expose arbitrary data (pass undefined or omit
result). Replace the current direct pass-through in each case with these
validated/coerced values so malformed UI payloads cannot change the expected
types.
In `@src/shared/flow/interfaces/browser/prompts.ts`:
- Around line 20-25: The confirmPrompt signature currently uses result: any
which allows invalid prompt completions; change the type in the interface
confirmPrompt (in src/shared/flow/interfaces/browser/prompts.ts) from any to the
tightened union string | boolean | null | undefined (or a discriminated union)
and then add validation at the IPC boundary where promptCompleted is forwarded
(preload/index.ts around promptCompleted usage and main ipc handler in
src/main/ipc/browser/prompts/browser.ts) to reject/throw or return an error for
values that don’t match those types; ensure callers that forward
window.confirm/window.prompt results are updated to coerce/guard to
string|boolean|null|undefined before calling confirmPrompt and update any
tests/types that relied on any.
In `@src/shared/utility.ts`:
- Around line 3-8: The helper in src/shared/utility.ts that computes the
suppression key currently returns urlObject.hostname for http/https which
collapses scheme/port; change it to always return urlObject.origin so
suppression is keyed by full origin (scheme + host + port). Update the function
that builds suppressionKey (referenced where suppressionKey is used in
src/main/ipc/browser/prompts/page.ts) to consume this origin value; if the UI
needs a hostname-only display, add/use a separate display helper (e.g.,
getDisplayHost or similar) rather than changing the suppression key logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 021dc531-25ec-4d2b-896e-e654efaad469
📒 Files selected for processing (9)
src/main/ipc/browser/prompts/browser.tssrc/main/ipc/browser/prompts/page.tssrc/main/modules/prompts.tssrc/preload/index.tssrc/renderer/src/components/browser-ui/web-prompts.tsxsrc/shared/flow/interfaces/browser/prompts.tssrc/shared/types/prompts.tssrc/shared/types/utils.tssrc/shared/utility.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/modules/prompts.ts (1)
77-105:⚠️ Potential issue | 🟠 MajorNormalize completion payloads before resolving the page.
promptCompleted()currently trusts IPC input. A malformed browser-UI call can makewindow.confirm()resolve to a string/object orwindow.alert()return something other thanundefined. Sanitize byactivePrompt.typeat this boundary, even if you also tighten the caller types.🛡️ Proposed fix
export function promptCompleted(promptId: string, result: any, suppress: boolean) { const activePrompt = removePromptById(activePrompts, promptId); if (!activePrompt) return false; @@ switch (activePrompt.type) { case "prompt": activePrompt.resolver({ success: true, - result + result: typeof result === "string" || result === null ? result : null }); break; case "confirm": activePrompt.resolver({ success: true, - result + result: result === true }); break; case "alert": activePrompt.resolver({ success: true, - result + result: undefined }); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/modules/prompts.ts` around lines 77 - 105, The promptCompleted function must normalize the completion payload before resolving to avoid malformed IPC input; update promptCompleted (and the branches that call activePrompt.resolver) to coerce result by activePrompt.type: for "confirm" convert to a boolean (e.g., Boolean(result)); for "alert" force result to undefined; for "prompt" ensure a string or null (e.g., if typeof result !== "string" set result = null); then pass the normalized value into activePrompt.resolver so all resolve payloads are type-safe.src/shared/flow/interfaces/browser/prompts.ts (1)
20-25:⚠️ Potential issue | 🟠 MajorRename and type this as a generic prompt-completion API.
confirmPrompt(promptId, result: any, suppress)is misleading because this path also resolvesalertand textprompt, andanyremoves the only compile-time guard against sending the wrong payload. This should be a neutral completion API (completePromptor similar) with a typed result such asstring | boolean | null | undefinedor a discriminated payload keyed by prompt type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/flow/interfaces/browser/prompts.ts` around lines 20 - 25, The current confirmPrompt method signature is misleading and untyped: confirmPrompt(promptId: string, result: any, suppress: boolean) — rename it to a neutral completion API (e.g., completePrompt) and replace the any result with a typed union or discriminated payload so callers and implementations get compile-time safety; for example change confirmPrompt to completePrompt(promptId: string, result: string | boolean | null | undefined, suppress?: boolean) or to completePrompt<T extends PromptResult>(promptId: string, result: PromptResultUnion, suppress?: boolean) (or use a discriminated union keyed by prompt type), then update all references/call sites and implementing classes/methods to use the new name and types (look for confirmPrompt usage and implementations of the interface to update accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/browser/prompts/browser.ts`:
- Around line 12-18: activePromptsChanged() and the prompts:get-active-prompts
path currently broadcast the full ActivePrompt[] to all renderers via
sendMessageToListeners and getActivePromptsForRenderer; instead, before sending,
filter the ActivePrompt[] down to only prompts owned by the target/listening
window (e.g., match by windowId/tabId/origin used elsewhere) and send that
filtered array only to that specific listener. Update activePromptsChanged (and
the prompts:get-active-prompts handler) to accept or derive the target listener
id/window id, call a helper like getActivePromptsForRenderer(targetWindowId)
that returns a filtered list, and replace the broadcast sendMessageToListeners
call with a per-listener send that transmits the scoped list only to that
listener.
---
Duplicate comments:
In `@src/main/modules/prompts.ts`:
- Around line 77-105: The promptCompleted function must normalize the completion
payload before resolving to avoid malformed IPC input; update promptCompleted
(and the branches that call activePrompt.resolver) to coerce result by
activePrompt.type: for "confirm" convert to a boolean (e.g., Boolean(result));
for "alert" force result to undefined; for "prompt" ensure a string or null
(e.g., if typeof result !== "string" set result = null); then pass the
normalized value into activePrompt.resolver so all resolve payloads are
type-safe.
In `@src/shared/flow/interfaces/browser/prompts.ts`:
- Around line 20-25: The current confirmPrompt method signature is misleading
and untyped: confirmPrompt(promptId: string, result: any, suppress: boolean) —
rename it to a neutral completion API (e.g., completePrompt) and replace the any
result with a typed union or discriminated payload so callers and
implementations get compile-time safety; for example change confirmPrompt to
completePrompt(promptId: string, result: string | boolean | null | undefined,
suppress?: boolean) or to completePrompt<T extends PromptResult>(promptId:
string, result: PromptResultUnion, suppress?: boolean) (or use a discriminated
union keyed by prompt type), then update all references/call sites and
implementing classes/methods to use the new name and types (look for
confirmPrompt usage and implementations of the interface to update accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4adb41e-48e5-40d2-9996-a5daf82876c7
📒 Files selected for processing (5)
src/main/ipc/browser/prompts/browser.tssrc/main/modules/prompts.tssrc/preload/index.tssrc/renderer/src/components/browser-ui/web-prompts.tsxsrc/shared/flow/interfaces/browser/prompts.ts
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.
window.prompt(),window.alert()andwindow.confirm()according to web specificationsflow.prompts.*APIs for browser UISummary by CodeRabbit
New Features
Chores
Dependencies