feat: crash report dialog with pre-filled GitHub issue URL - #732
Conversation
Replace bare dialog.showErrorBox in uncaughtException and unhandledRejection handlers with a two-button dialog offering to open a pre-filled GitHub issue. - New module: src/main/crash-report-dialog.ts - buildCrashReportUrl(): pre-fills template, title, platform, version, stack - showCrashReportDialog(): native sync dialog with Report/Dismiss buttons - 60s cooldown to prevent dialog spam from error loops - URL truncation to stay within browser/GitHub limits (~8000 chars) - No tokens, no telemetry — user controls submission via their own GH account - New issue template: .github/ISSUE_TEMPLATE/crash_report.md - Fields auto-populated by the crash reporter URL params - Includes privacy warning matching existing bug_report.md - Integration: src/main/index.ts - uncaughtException: showErrorBox → showCrashReportDialog - unhandledRejection: showErrorBox + inline cooldown → showCrashReportDialog - Keeps console.error + flushLogBeforeQuit before the dialog call - Tests: src/main/crash-report-dialog.test.ts (Vitest, node environment) - URL building, truncation, title limits - Dialog interaction (report, dismiss, cooldown, graceful failure)
📝 WalkthroughWalkthroughAdds a crash-report dialog that formats sanitized error details into a pre-filled GitHub issue URL, rate-limits prompts, integrates uncaught exceptions and rejected promises, and adds tests plus a GitHub crash-report issue template. ChangesCrash reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessError
participant CrashReportDialog
participant ElectronDialog
participant Browser
participant GitHub
ProcessError->>CrashReportDialog: showCrashReportDialog(ctx)
CrashReportDialog->>ElectronDialog: showMessageBoxSync(options)
ElectronDialog-->>CrashReportDialog: Report on GitHub
CrashReportDialog->>Browser: openExternal(issue URL)
Browser->>GitHub: Load pre-filled crash issue
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
src/main/crash-report-dialog.ts (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
.replace(/\n/g, ' ')— dead code.
sanitizeLogMessagealready replaces\x00-\x1F(which includes\n/\r) and then collapses all\s+to a single space, so by the time this line runs there are no newlines left to replace. SonarCloud also flags this line forreplaceAllvsreplace; either way the extra.replace()call is unreachable and can simply be dropped.🧹 Proposed cleanup
function formatErrorForTitle(ctx: CrashContext): string { const msg = ctx.error instanceof Error ? ctx.error.message : String(ctx.error); - const cleaned = sanitizeLogMessage(msg).replace(/\n/g, ' ').slice(0, 80); + const cleaned = sanitizeLogMessage(msg).slice(0, 80); return `[Crash] ${cleaned}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/crash-report-dialog.ts` around lines 52 - 56, Remove the redundant `.replace(/\n/g, ' ')` call from `formatErrorForTitle`; use the sanitized result directly before applying the 80-character slice and title prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/crash-report-dialog.test.ts`:
- Around line 186-198: Reset the crash-dialog cooldown between the two
showCrashReportDialog invocations in handles dialog unavailable gracefully by
calling resetCrashDialogCooldownForTests(), ensuring the second assertion
reaches the mocked showMessageBoxSync throw path rather than being skipped by
the cooldown gate.
In `@src/main/crash-report-dialog.ts`:
- Around line 35-41: Add the required `// catch-no-log-ok <reason>` annotation
to the `catch` block in `getAppVersion`, documenting why the version lookup
failure intentionally returns `'unknown'` without logging; preserve the existing
fallback behavior.
- Around line 58-102: Update formatErrorForBody to use sanitizeForBody for both
the error message and stack trace, preserving stack-trace line breaks inside the
code fence instead of using sanitizeLogMessage. Also add the missing warning
after the diagnostic bundle instructions telling users not to attach Export for
Developer or mesh-client.db because it may contain saved passwords.
- Around line 108-135: Update buildCrashReportUrl so truncation measures the
URL-encoded body rather than subtracting raw body.length from the encoded URL
length. Rebuild the URL after truncation and continue reducing the body, if
necessary, until the final URL is at or below MAX_URL_LENGTH.
In `@src/main/index.ts`:
- Around line 470-477: Update the unhandledRejection handler to await
flushLogBeforeQuit() before calling showCrashReportDialog, ensuring the log
flush settles before the blocking dialog appears; apply the same sequencing
change to the uncaughtException handler.
---
Nitpick comments:
In `@src/main/crash-report-dialog.ts`:
- Around line 52-56: Remove the redundant `.replace(/\n/g, ' ')` call from
`formatErrorForTitle`; use the sanitized result directly before applying the
80-character slice and title prefix.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fcf3b2c-ec0b-48ba-8001-5d086a37657e
📒 Files selected for processing (4)
.github/ISSUE_TEMPLATE/crash_report.mdsrc/main/crash-report-dialog.test.tssrc/main/crash-report-dialog.tssrc/main/index.ts
| it('handles dialog unavailable gracefully', () => { | ||
| mockShowMessageBoxSync.mockImplementation(() => { | ||
| throw new Error('dialog unavailable'); | ||
| }); | ||
|
|
||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error('test'), | ||
| }; | ||
|
|
||
| expect(() => showCrashReportDialog(ctx)).not.toThrow(); | ||
| expect(showCrashReportDialog(ctx)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Second showCrashReportDialog call doesn't exercise the throw path.
Cooldown state (lastCrashDialogAt) isn't reset between the two calls in this test, so the second call at line 197 is short-circuited by the 60s cooldown gate (line 149-151) before it ever reaches the mocked showMessageBoxSync that throws. The assertion still passes, but it isn't verifying repeated graceful handling of a throwing dialog — call resetCrashDialogCooldownForTests() between the two invocations if that's the intent.
✅ Proposed fix
expect(() => showCrashReportDialog(ctx)).not.toThrow();
+ resetCrashDialogCooldownForTests();
expect(showCrashReportDialog(ctx)).toBe(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('handles dialog unavailable gracefully', () => { | |
| mockShowMessageBoxSync.mockImplementation(() => { | |
| throw new Error('dialog unavailable'); | |
| }); | |
| const ctx: CrashContext = { | |
| source: 'uncaughtException', | |
| error: new Error('test'), | |
| }; | |
| expect(() => showCrashReportDialog(ctx)).not.toThrow(); | |
| expect(showCrashReportDialog(ctx)).toBe(false); | |
| }); | |
| it('handles dialog unavailable gracefully', () => { | |
| mockShowMessageBoxSync.mockImplementation(() => { | |
| throw new Error('dialog unavailable'); | |
| }); | |
| const ctx: CrashContext = { | |
| source: 'uncaughtException', | |
| error: new Error('test'), | |
| }; | |
| expect(() => showCrashReportDialog(ctx)).not.toThrow(); | |
| resetCrashDialogCooldownForTests(); | |
| expect(showCrashReportDialog(ctx)).toBe(false); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/crash-report-dialog.test.ts` around lines 186 - 198, Reset the
crash-dialog cooldown between the two showCrashReportDialog invocations in
handles dialog unavailable gracefully by calling
resetCrashDialogCooldownForTests(), ensuring the second assertion reaches the
mocked showMessageBoxSync throw path rather than being skipped by the cooldown
gate.
| function getAppVersion(): string { | ||
| try { | ||
| return app.getVersion(); | ||
| } catch { | ||
| return 'unknown'; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing catch-no-log-ok annotation.
This catch block neither logs, rethrows, nor carries the required annotation comment, unlike the other two catches in this same file (lines 178-180, 183-185) which correctly include one.
As per coding guidelines, "Catches must log, rethrow, or include // catch-no-log-ok <reason>; use console.debug, console.warn, or console.error, never bare console.log."
🩹 Proposed fix
function getAppVersion(): string {
try {
return app.getVersion();
- } catch {
+ } catch {
+ // catch-no-log-ok app.getVersion() rarely throws; version is cosmetic only
return 'unknown';
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getAppVersion(): string { | |
| try { | |
| return app.getVersion(); | |
| } catch { | |
| return 'unknown'; | |
| } | |
| } | |
| function getAppVersion(): string { | |
| try { | |
| return app.getVersion(); | |
| } catch { | |
| // catch-no-log-ok app.getVersion() rarely throws; version is cosmetic only | |
| return 'unknown'; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/crash-report-dialog.ts` around lines 35 - 41, Add the required `//
catch-no-log-ok <reason>` annotation to the `catch` block in `getAppVersion`,
documenting why the version lookup failure intentionally returns `'unknown'`
without logging; preserve the existing fallback behavior.
Source: Coding guidelines
| function formatErrorForBody(ctx: CrashContext): string { | ||
| const msg = ctx.error instanceof Error ? ctx.error.message : String(ctx.error); | ||
| const stack = | ||
| ctx.error instanceof Error && ctx.error.stack | ||
| ? ctx.error.stack.slice(0, MAX_STACK_LENGTH) | ||
| : '(no stack trace)'; | ||
|
|
||
| const platform = getPlatformLabel(); | ||
| const version = getAppVersion(); | ||
| const arch = process.arch; | ||
| const os = osRelease(); | ||
| const packaged = app.isPackaged ? 'yes' : 'no (dev)'; | ||
|
|
||
| return [ | ||
| '**Crash source:** `' + ctx.source + '`', | ||
| '', | ||
| '**Desktop:**', | ||
| `- OS: ${platform} ${os} (${arch})`, | ||
| `- App version: ${version}`, | ||
| `- Packaged: ${packaged}`, | ||
| '', | ||
| '**Error message:**', | ||
| '```', | ||
| sanitizeLogMessage(msg), | ||
| '```', | ||
| '', | ||
| '**Stack trace:**', | ||
| '```', | ||
| sanitizeLogMessage(stack), | ||
| '```', | ||
| '', | ||
| '---', | ||
| '', | ||
| '**Diagnostic bundle:**', | ||
| 'Please also attach the zip from **App → Support / Bug reports → Export for GitHub** if the app is still responsive.', | ||
| '', | ||
| '**Steps to reproduce (please fill in):**', | ||
| '1. ', | ||
| '2. ', | ||
| '3. ', | ||
| '', | ||
| '**Additional context:**', | ||
| '', | ||
| ].join('\n'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
sanitizeLogMessage collapses the stack trace into a single line, and the "do not attach password DB" warning from the template is dropped.
Two separate issues in this function:
- Stack trace loses all line breaks (line 86).
sanitizeLogMessagestrips\x00-\x1F(including\n/\r) and collapses remaining whitespace to single spaces — it's built for single-line log entries. Applying it to the multi-line stack trace inside the ``` code fence turns the entire stack into one unreadable wall of text, defeating the purpose of including it. Note the test suite mockssanitizeLogMessageas an identity function, so this regression isn't caught by any test. - Missing password-DB warning (compare with
.github/ISSUE_TEMPLATE/crash_report.mdline 33). The template explicitly warns:**Do not** attach **Export for Developer** or \mesh-client.db`... may contain saved passwords. Since GitHub's?body=` query param completely overrides the template body, this warning never reaches the user through the auto-filled crash report flow — exactly the scenario where a user might attach the wrong export.
🛠️ Proposed fix
+function sanitizeForBody(text: string): string {
+ // Strip dangerous control chars but preserve newlines so multi-line
+ // content (stack traces) stays readable in the GitHub issue body.
+ return text
+ .replace(/[\x00-\x09\x0B\x0C\x0E-\x1F\x7F\u2028\u2029]+/g, ' ')
+ .replace(/[ \t]+/g, ' ')
+ .trim();
+}
+
function formatErrorForBody(ctx: CrashContext): string {
...
return [
...
'**Error message:**',
'```',
- sanitizeLogMessage(msg),
+ sanitizeForBody(msg),
'```',
'',
'**Stack trace:**',
'```',
- sanitizeLogMessage(stack),
+ sanitizeForBody(stack),
'```',
'',
'---',
'',
'**Diagnostic bundle:**',
'Please also attach the zip from **App → Support / Bug reports → Export for GitHub** if the app is still responsive.',
+ '',
+ '**Do not** attach **Export for Developer** or `mesh-client.db` to this public issue — the database may contain saved passwords.',
'',
...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function formatErrorForBody(ctx: CrashContext): string { | |
| const msg = ctx.error instanceof Error ? ctx.error.message : String(ctx.error); | |
| const stack = | |
| ctx.error instanceof Error && ctx.error.stack | |
| ? ctx.error.stack.slice(0, MAX_STACK_LENGTH) | |
| : '(no stack trace)'; | |
| const platform = getPlatformLabel(); | |
| const version = getAppVersion(); | |
| const arch = process.arch; | |
| const os = osRelease(); | |
| const packaged = app.isPackaged ? 'yes' : 'no (dev)'; | |
| return [ | |
| '**Crash source:** `' + ctx.source + '`', | |
| '', | |
| '**Desktop:**', | |
| `- OS: ${platform} ${os} (${arch})`, | |
| `- App version: ${version}`, | |
| `- Packaged: ${packaged}`, | |
| '', | |
| '**Error message:**', | |
| '```', | |
| sanitizeLogMessage(msg), | |
| '```', | |
| '', | |
| '**Stack trace:**', | |
| '```', | |
| sanitizeLogMessage(stack), | |
| '```', | |
| '', | |
| '---', | |
| '', | |
| '**Diagnostic bundle:**', | |
| 'Please also attach the zip from **App → Support / Bug reports → Export for GitHub** if the app is still responsive.', | |
| '', | |
| '**Steps to reproduce (please fill in):**', | |
| '1. ', | |
| '2. ', | |
| '3. ', | |
| '', | |
| '**Additional context:**', | |
| '', | |
| ].join('\n'); | |
| } | |
| function sanitizeForBody(text: string): string { | |
| // Strip dangerous control chars but preserve newlines so multi-line | |
| // content (stack traces) stays readable in the GitHub issue body. | |
| return text | |
| .replace(/[\x00-\x09\x0B\x0C\x0E-\x1F\x7F\u2028\u2029]+/g, ' ') | |
| .replace(/[ \t]+/g, ' ') | |
| .trim(); | |
| } | |
| function formatErrorForBody(ctx: CrashContext): string { | |
| const msg = ctx.error instanceof Error ? ctx.error.message : String(ctx.error); | |
| const stack = | |
| ctx.error instanceof Error && ctx.error.stack | |
| ? ctx.error.stack.slice(0, MAX_STACK_LENGTH) | |
| : '(no stack trace)'; | |
| const platform = getPlatformLabel(); | |
| const version = getAppVersion(); | |
| const arch = process.arch; | |
| const os = osRelease(); | |
| const packaged = app.isPackaged ? 'yes' : 'no (dev)'; | |
| return [ | |
| '**Crash source:** `' + ctx.source + '`', | |
| '', | |
| '**Desktop:**', | |
| `- OS: ${platform} ${os} (${arch})`, | |
| `- App version: ${version}`, | |
| `- Packaged: ${packaged}`, | |
| '', | |
| '**Error message:**', | |
| ' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/crash-report-dialog.ts` around lines 58 - 102, Update
formatErrorForBody to use sanitizeForBody for both the error message and stack
trace, preserving stack-trace line breaks inside the code fence instead of using
sanitizeLogMessage. Also add the missing warning after the diagnostic bundle
instructions telling users not to attach Export for Developer or mesh-client.db
because it may contain saved passwords.
| export function buildCrashReportUrl(ctx: CrashContext): string { | ||
| const title = formatErrorForTitle(ctx); | ||
| let body = formatErrorForBody(ctx); | ||
|
|
||
| const baseUrl = `https://github.com/${REPO_OWNER}/${REPO_NAME}/issues/new`; | ||
| const params = new URLSearchParams({ | ||
| template: ISSUE_TEMPLATE, | ||
| title, | ||
| body, | ||
| }); | ||
|
|
||
| let url = `${baseUrl}?${params.toString()}`; | ||
|
|
||
| if (url.length > MAX_URL_LENGTH) { | ||
| const overhead = url.length - body.length; | ||
| const maxBody = MAX_URL_LENGTH - overhead - 100; | ||
| body = | ||
| body.slice(0, maxBody) + '\n\n_(truncated — attach Export for GitHub zip for full details)_'; | ||
| const truncatedParams = new URLSearchParams({ | ||
| template: ISSUE_TEMPLATE, | ||
| title, | ||
| body, | ||
| }); | ||
| url = `${baseUrl}?${truncatedParams.toString()}`; | ||
| } | ||
|
|
||
| return url; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does URLSearchParams.toString() percent-encode newlines and backticks in Node.js?
💡 Result:
Yes, the URLSearchParams.toString method in Node.js percent-encodes both newlines and backticks, along with most other characters that are not ASCII alphanumeric or specific "unreserved" characters [1][2]. The URLSearchParams API adheres to the WHATWG URL specification, which uses the application/x-www-form-urlencoded percent-encode set [1][3]. According to this specification, this encode set includes all code points except for: - ASCII alphanumeric characters (a-z, A-Z, 0-9) [1][2] - The asterisk (*) [1][2] - The hyphen (-) [1][2] - The period (.) [1][2] - The underscore (_) [1][2] Because newlines (U+000A) and backticks (U+0060) are not in this list of allowed characters, they are percent-encoded by the toString method [1][2]. Additionally, while URLSearchParams encodes spaces as a plus sign (+) [1][2][4], other non-alphanumeric characters are typically percent-encoded as a percentage sign followed by their hexadecimal character code (e.g., a newline might be encoded as %0A and a backtick as %60).
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/toString
- 2: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams
- 3: https://stackoverflow.com/questions/59889140/different-output-from-encodeuricomponent-vs-urlsearchparams
- 4: https://dev.to/mahdavipanah/pitfalls-of-url-and-urlsearchparams-in-javascript-4ef8
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/main/crash-report-dialog.ts | cat -nRepository: Colorado-Mesh/mesh-client
Length of output: 7154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the function and nearby constants
sed -n '1,220p' src/main/crash-report-dialog.ts | cat -nRepository: Colorado-Mesh/mesh-client
Length of output: 7154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'JS'
const MAX_URL_LENGTH = 8000;
const baseUrl = 'https://github.com/Colorado-Mesh/mesh-client/issues/new';
const title = '[Crash] ' + 'x'.repeat(80);
const template = 'crash_report.md';
const suffix = '\n\n_(truncated — attach Export for GitHub zip for full details)_';
function makeUrl(body) {
const params = new URLSearchParams({ template, title, body });
return `${baseUrl}?${params.toString()}`;
}
function build(body) {
let url = makeUrl(body);
if (url.length > MAX_URL_LENGTH) {
const overhead = url.length - body.length;
const maxBody = MAX_URL_LENGTH - overhead - 100;
const truncatedBody = body.slice(0, maxBody) + suffix;
url = makeUrl(truncatedBody);
}
return url;
}
const chars = [
['a', 'ASCII'],
['\n', 'newline'],
['€', 'euro'],
['🙂', 'emoji'],
['`', 'backtick'],
['(', 'paren'],
['—', 'emdash'],
];
for (const [ch, label] of chars) {
let worst = null;
for (let a = 0; a <= 3000; a += 100) {
for (let b = 0; b <= 7000; b += 100) {
const body = ch.repeat(a) + 'a'.repeat(b);
const url = makeUrl(body);
if (url.length <= MAX_URL_LENGTH) continue;
const out = build(body);
if (out.length > MAX_URL_LENGTH) {
worst = { label, a, b, inLen: url.length, outLen: out.length };
console.log('FOUND', JSON.stringify(worst));
process.exit(0);
}
}
}
}
console.log('No overflow found in coarse grid.');
JSRepository: Colorado-Mesh/mesh-client
Length of output: 230
Recompute the limit after encoding. overhead mixes encoded url.length with raw body.length, so a body with lots of multibyte/percent-encoded characters can still produce a final URL over MAX_URL_LENGTH after truncation. Recalculate against the encoded body (or loop until the rebuilt URL fits).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/crash-report-dialog.ts` around lines 108 - 135, Update
buildCrashReportUrl so truncation measures the URL-encoded body rather than
subtracting raw body.length from the encoded URL length. Rebuild the URL after
truncation and continue reducing the body, if necessary, until the final URL is
at or below MAX_URL_LENGTH.
| process.on('unhandledRejection', (reason) => { | ||
| console.error( | ||
| '[main] Unhandled rejection:', | ||
| sanitizeLogMessage(reason instanceof Error ? (reason.stack ?? reason.message) : String(reason)), | ||
| ); | ||
| void flushLogBeforeQuit(); | ||
| const now = Date.now(); | ||
| if (now - lastUnhandledRejectionDialogAt < UNHANDLED_REJECTION_DIALOG_COOLDOWN_MS) return; | ||
| lastUnhandledRejectionDialogAt = now; | ||
| const message = | ||
| reason instanceof Error ? `${reason.message}\n\n${reason.stack ?? ''}` : String(reason); | ||
| try { | ||
| dialog.showErrorBox( | ||
| 'Mesh-Client — Unhandled Promise Rejection', | ||
| `A promise rejected without a handler. Check the main process terminal for full details.\n\n${message.slice(0, 1500)}${message.length > 1500 ? '…' : ''}`, | ||
| ); | ||
| } catch { | ||
| // catch-no-log-ok dialog unavailable during early startup; rejection already logged above | ||
| } | ||
| const error = reason instanceof Error ? reason : new Error(String(reason)); | ||
| showCrashReportDialog({ source: 'unhandledRejection', error }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Electron dialog.showMessageBoxSync block the Node.js event loop until dismissed?
💡 Result:
Yes, the electron dialog.showMessageBoxSync method blocks the process until the message box is closed [1][2]. Because Electron's main process runs on the Node.js event loop, this synchronous blocking behavior prevents the event loop from processing other tasks, such as handling IPC messages or updating windows, until the dialog is dismissed [3][4]. {value: "Yes, the electron dialog.showMessageBoxSync method blocks the process until the message box is closed [1][2]. Because Electron's main process runs on the Node.js event loop, this synchronous blocking behavior prevents the event loop from processing other tasks, such as handling IPC messages or updating windows, until the dialog is dismissed [3][4].", citations: [1][2][3][4], confidence: 1.0}
Citations:
- 1: https://github.com/electron/electron/blob/main/docs/api/dialog.md
- 2: https://electronjs.org/docs/latest/api/dialog
- 3: Async method
dialog.showMessageBoxis blocking electron/electron#23319 - 4: Calling dialog.showMessageBox on a parent window immediately after the closure of a modal window freezes the parent window electron/electron#50068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate the functions mentioned in the comment.
ast-grep outline src/main/index.ts --view expanded || true
rg -n "flushLogBeforeQuit|showCrashReportDialog|showMessageBoxSync|unhandledRejection|uncaughtException|appendChain" src/main/index.ts src/main -S || true
# Read the relevant section around the handlers.
sed -n '430,520p' src/main/index.ts
# Find the flush implementation.
rg -n "function flushLogBeforeQuit|const flushLogBeforeQuit|flushLogBeforeQuit =" src/main -SRepository: Colorado-Mesh/mesh-client
Length of output: 18188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the log flush implementation and crash dialog behavior.
sed -n '1,240p' src/main/log-service.ts
printf '\n--- crash-report-dialog ---\n'
sed -n '1,240p' src/main/crash-report-dialog.tsRepository: Colorado-Mesh/mesh-client
Length of output: 13900
Await flushLogBeforeQuit() before showing the crash dialog. showMessageBoxSync() blocks the main process, so the fire-and-forget flush won’t settle before the dialog appears. The same pattern exists in uncaughtException.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/index.ts` around lines 470 - 477, Update the unhandledRejection
handler to await flushLogBeforeQuit() before calling showCrashReportDialog,
ensuring the log flush settles before the blocking dialog appears; apply the
same sequencing change to the uncaughtException handler.
Source: Coding guidelines
1 participant
Summary
Replaces bare
dialog.showErrorBoxin theuncaughtExceptionandunhandledRejectionhandlers with a two-button native dialog offering to open a pre-filled GitHub issue in the user's browser.Before: Error dialog shows crash text with no actionable next step.
After: User sees "Report on GitHub" / "Dismiss" — clicking Report opens their browser with issue template, platform info, error message, and stack trace pre-filled.
Design decisions
?template=crash_report.md&title=...&body=...query params (truncated to ~8000 chars for browser safety).showMessageBoxSync— synchronous becauseuncaughtExceptionis a sync context that may exit immediately after.lastUnhandledRejectionDialogAtpattern).dialogis unavailable (early startup, after quit), catches silently.What changed
src/main/crash-report-dialog.tssrc/main/crash-report-dialog.test.ts.github/ISSUE_TEMPLATE/crash_report.mdsrc/main/index.tsWhat it looks like for the user
Testing
throw new Error('test')in main process to verify dialog + URLNotes
console.error+flushLogBeforeQuit()calls preserved — crash data always on diskrender-process-gonelistener using the same APISummary by CodeRabbit
New Features
Documentation