Skip to content

fix: Redefine window.confirm() in preload#307

Open
ajinkya-browserstack wants to merge 1 commit intomasterfrom
RQ-799-preload-window-confirm
Open

fix: Redefine window.confirm() in preload#307
ajinkya-browserstack wants to merge 1 commit intomasterfrom
RQ-799-preload-window-confirm

Conversation

@ajinkya-browserstack
Copy link
Contributor

@ajinkya-browserstack ajinkya-browserstack commented Mar 13, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Improved Windows confirmation dialogs by switching to native system dialogs, resolving focus and input caret issues previously experienced in the application.

@coderabbitai
Copy link

coderabbitai bot commented Mar 13, 2026

Walkthrough

This change modifies src/main/preload.js to override the global window.confirm function on Windows platforms. The dialog module is imported from @electron/remote and used to replace the browser's native confirm dialog with a call to dialog.showMessageBoxSync. The override presents a native dialog with OK/Cancel buttons, returning true for OK selection and false for Cancel. Error handling includes fallback to the original confirm function if the dialog call fails, with the entire override wrapped in try/catch to prevent preload-time errors. The change is Windows-specific and does not affect non-Windows platforms.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: redefining window.confirm() in the preload script to fix focus/caret issues on Windows by using native dialogs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch RQ-799-preload-window-confirm
📝 Coding Plan
  • Generate coding plan for human review comments

Warning

Tools execution failed with the following error:

Failed to run tools: 14 UNAVAILABLE: read ECONNRESET


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can generate walkthrough in a markdown collapsible section to save space.

Enable the reviews.collapse_walkthrough setting to generate walkthrough in a markdown collapsible section.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/preload.js`:
- Around line 47-49: The fallback in the catch block inside the confirm shim
currently returns true when both showMessageBoxSync and originalConfirm are
unavailable, which auto-confirms on error; change the catch to return
originalConfirm ? originalConfirm(message) : false (or otherwise explicitly
deny) and optionally log the caught error so failures do not grant implicit
consent; update the catch that references showMessageBoxSync and originalConfirm
in preload.js accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2bdb2e23-46cc-42db-b33a-d51d5a6dea38

📥 Commits

Reviewing files that changed from the base of the PR and between f5d6154 and 1622fec.

📒 Files selected for processing (1)
  • src/main/preload.js

Comment on lines +47 to +49
} catch (e) {
// Fallback to original confirm if dialog fails for any reason
return originalConfirm ? originalConfirm(message) : true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Do not auto-confirm on fallback failure.

Line 49 returns true when both showMessageBoxSync and originalConfirm are unavailable. That turns a dialog error into implicit user consent and can execute destructive flows without confirmation.

🛠️ Proposed fix
-        return originalConfirm ? originalConfirm(message) : true;
+        return originalConfirm ? originalConfirm.call(window, message) : false;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/preload.js` around lines 47 - 49, The fallback in the catch block
inside the confirm shim currently returns true when both showMessageBoxSync and
originalConfirm are unavailable, which auto-confirms on error; change the catch
to return originalConfirm ? originalConfirm(message) : false (or otherwise
explicitly deny) and optionally log the caught error so failures do not grant
implicit consent; update the catch that references showMessageBoxSync and
originalConfirm in preload.js accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant