-
-
Notifications
You must be signed in to change notification settings - Fork 11
feat: crash report dialog with pre-filled GitHub issue URL #732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Letark
wants to merge
1
commit into
main
Choose a base branch
from
feat/crash-report-dialog
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+451
−25
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| --- | ||
| name: Crash report | ||
| about: Automatically generated crash report from the app | ||
| title: '' | ||
| labels: 'crash' | ||
| assignees: '' | ||
| --- | ||
|
|
||
| <!-- This template is used by the in-app crash reporter. Fields below are pre-filled automatically. --> | ||
|
|
||
| **Crash source:** `(filled by crash reporter)` | ||
|
|
||
| **Desktop:** | ||
| - OS: | ||
| - App version: | ||
| - Packaged: | ||
|
|
||
| **Error message:** | ||
| ``` | ||
| (filled by crash reporter) | ||
| ``` | ||
|
|
||
| **Stack trace:** | ||
| ``` | ||
| (filled by crash reporter) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| **Diagnostic bundle:** | ||
| Please 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. | ||
|
|
||
| **Steps to reproduce (please fill in):** | ||
| 1. | ||
| 2. | ||
| 3. | ||
|
|
||
| **Additional 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| // @vitest-environment node | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { | ||
| buildCrashReportUrl, | ||
| type CrashContext, | ||
| resetCrashDialogCooldownForTests, | ||
| showCrashReportDialog, | ||
| } from './crash-report-dialog'; | ||
|
|
||
| const mockShowMessageBoxSync = vi.fn().mockReturnValue(1); // default: Dismiss | ||
| const mockOpenExternal = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| vi.mock('electron', () => ({ | ||
| app: { | ||
| getVersion: () => '5.24.1', | ||
| isPackaged: true, | ||
| }, | ||
| dialog: { | ||
| showMessageBoxSync: (...args: unknown[]) => mockShowMessageBoxSync(...args), | ||
| }, | ||
| shell: { | ||
| openExternal: (...args: unknown[]) => mockOpenExternal(...args), | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock('./sanitize-log-message', () => ({ | ||
| sanitizeLogMessage: (msg: string) => msg, | ||
| })); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| resetCrashDialogCooldownForTests(); | ||
| }); | ||
|
|
||
| describe('buildCrashReportUrl', () => { | ||
| it('builds a valid GitHub issue URL with crash context', () => { | ||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error('Cannot read properties of null'), | ||
| }; | ||
|
|
||
| const url = buildCrashReportUrl(ctx); | ||
|
|
||
| expect(url).toContain('https://github.com/Colorado-Mesh/mesh-client/issues/new'); | ||
| expect(url).toContain('template=crash_report.md'); | ||
| expect(url).toContain('Cannot+read+properties+of+null'); | ||
| expect(url).toContain('uncaughtException'); | ||
| expect(url).toContain('5.24.1'); | ||
| }); | ||
|
|
||
| it('includes platform and architecture info', () => { | ||
| const ctx: CrashContext = { | ||
| source: 'unhandledRejection', | ||
| error: new Error('ENOENT'), | ||
| }; | ||
|
|
||
| const url = decodeURIComponent(buildCrashReportUrl(ctx)); | ||
|
|
||
| expect(url).toContain(`App version: 5.24.1`); | ||
| expect(url).toContain('Packaged: yes'); | ||
| expect(url).toContain(process.arch); | ||
| }); | ||
|
|
||
| it('handles string errors', () => { | ||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: 'raw string error', | ||
| }; | ||
|
|
||
| const url = buildCrashReportUrl(ctx); | ||
|
|
||
| expect(url).toContain('raw+string+error'); | ||
| expect(url).toContain('no+stack+trace'); | ||
| }); | ||
|
|
||
| it('truncates URLs exceeding the max length', () => { | ||
| const longMessage = 'x'.repeat(10_000); | ||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error(longMessage), | ||
| }; | ||
|
|
||
| const url = buildCrashReportUrl(ctx); | ||
|
|
||
| expect(url.length).toBeLessThanOrEqual(8200); // allow slight encoding overhead | ||
| expect(url).toContain('truncated'); | ||
| }); | ||
|
|
||
| it('truncates title to 80 chars', () => { | ||
| const longMessage = 'A'.repeat(200); | ||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error(longMessage), | ||
| }; | ||
|
|
||
| const url = buildCrashReportUrl(ctx); | ||
| const params = new URLSearchParams(url.split('?')[1]); | ||
| const title = params.get('title') ?? ''; | ||
|
|
||
| // [Crash] + space + 80 chars = 88 max | ||
| expect(title.length).toBeLessThanOrEqual(88); | ||
| }); | ||
| }); | ||
|
|
||
| describe('showCrashReportDialog', () => { | ||
| it('shows dialog and returns false when user dismisses', () => { | ||
| mockShowMessageBoxSync.mockReturnValue(1); | ||
|
|
||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error('test error'), | ||
| }; | ||
|
|
||
| const result = showCrashReportDialog(ctx); | ||
|
|
||
| expect(result).toBe(false); | ||
| expect(mockShowMessageBoxSync).toHaveBeenCalledOnce(); | ||
| expect(mockOpenExternal).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('opens browser and returns true when user clicks Report', () => { | ||
| mockShowMessageBoxSync.mockReturnValue(0); | ||
|
|
||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error('test error'), | ||
| }; | ||
|
|
||
| const result = showCrashReportDialog(ctx); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(mockOpenExternal).toHaveBeenCalledOnce(); | ||
| expect(mockOpenExternal.mock.calls[0][0]).toContain( | ||
| 'https://github.com/Colorado-Mesh/mesh-client/issues/new', | ||
| ); | ||
| }); | ||
|
|
||
| it('respects 60s cooldown between dialogs', () => { | ||
| mockShowMessageBoxSync.mockReturnValue(1); | ||
|
|
||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error('first'), | ||
| }; | ||
|
|
||
| showCrashReportDialog(ctx); | ||
| const secondResult = showCrashReportDialog(ctx); | ||
|
|
||
| expect(secondResult).toBe(false); | ||
| expect(mockShowMessageBoxSync).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| it('shows dialog again after cooldown resets', () => { | ||
| mockShowMessageBoxSync.mockReturnValue(1); | ||
|
|
||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error('test'), | ||
| }; | ||
|
|
||
| showCrashReportDialog(ctx); | ||
| resetCrashDialogCooldownForTests(); | ||
| showCrashReportDialog(ctx); | ||
|
|
||
| expect(mockShowMessageBoxSync).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it('passes error type in dialog options', () => { | ||
| mockShowMessageBoxSync.mockReturnValue(1); | ||
|
|
||
| const ctx: CrashContext = { | ||
| source: 'uncaughtException', | ||
| error: new Error('something broke'), | ||
| }; | ||
|
|
||
| showCrashReportDialog(ctx); | ||
|
|
||
| const options = mockShowMessageBoxSync.mock.calls[0][0] as Record<string, unknown>; | ||
| expect(options.type).toBe('error'); | ||
| expect(options.buttons).toEqual(['Report on GitHub', 'Dismiss']); | ||
| expect(options.detail).toContain('something broke'); | ||
| expect(options.detail).toContain('uncaughtException'); | ||
| }); | ||
|
|
||
| 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 string errors in dialog detail', () => { | ||
| mockShowMessageBoxSync.mockReturnValue(1); | ||
|
|
||
| const ctx: CrashContext = { | ||
| source: 'unhandledRejection', | ||
| error: 'non-Error rejection value', | ||
| }; | ||
|
|
||
| showCrashReportDialog(ctx); | ||
|
|
||
| const options = mockShowMessageBoxSync.mock.calls[0][0] as Record<string, unknown>; | ||
| expect(options.detail).toContain('non-Error rejection value'); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Second
showCrashReportDialogcall 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 mockedshowMessageBoxSyncthat throws. The assertion still passes, but it isn't verifying repeated graceful handling of a throwing dialog — callresetCrashDialogCooldownForTests()between the two invocations if that's the intent.✅ Proposed fix
expect(() => showCrashReportDialog(ctx)).not.toThrow(); + resetCrashDialogCooldownForTests(); expect(showCrashReportDialog(ctx)).toBe(false);📝 Committable suggestion
🤖 Prompt for AI Agents