From 558abd63dca2d32d572f91c68273dc54997c20fa Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 16 May 2026 17:54:52 -0700 Subject: [PATCH] fix: address review findings --- .../crash-report-store.test.ts | 11 ++++++++ .../crash-reporting/crash-report-store.ts | 14 +++++++++- src/main/ipc/crash-reporting.test.ts | 28 +++++++++++++++++++ src/main/ipc/crash-reporting.ts | 17 ++++++++++- .../crash-report/CrashReportDialog.tsx | 26 +++++++++++++++-- 5 files changed, 91 insertions(+), 5 deletions(-) diff --git a/src/main/crash-reporting/crash-report-store.test.ts b/src/main/crash-reporting/crash-report-store.test.ts index 5a34b4e57d5..44e6c83fdc6 100644 --- a/src/main/crash-reporting/crash-report-store.test.ts +++ b/src/main/crash-reporting/crash-report-store.test.ts @@ -87,6 +87,17 @@ describe('CrashReportStore', () => { await expect(store.markSent(report.id)).resolves.toMatchObject({ status: 'dismissed' }) }) + it('persists a submitted dismissed report as sent', async () => { + const { store, filePath } = await createStore() + const report = await store.record(input()) + + await expect(store.dismiss(report.id)).resolves.toMatchObject({ status: 'dismissed' }) + await expect(store.markDismissedSent(report.id)).resolves.toMatchObject({ status: 'sent' }) + + const reloaded = new CrashReportStore(filePath) + await expect(reloaded.getById(report.id)).resolves.toMatchObject({ status: 'sent' }) + }) + it('serializes concurrent writes', async () => { const { store } = await createStore() diff --git a/src/main/crash-reporting/crash-report-store.ts b/src/main/crash-reporting/crash-report-store.ts index 03b04e7b4ae..695ecca571a 100644 --- a/src/main/crash-reporting/crash-report-store.ts +++ b/src/main/crash-reporting/crash-report-store.ts @@ -57,6 +57,10 @@ export class CrashReportStore { return this.transitionPending(id, 'sent') } + async markDismissedSent(id: string): Promise { + return this.transitionStatus(id, 'dismissed', 'sent') + } + async dismiss(id: string): Promise { return this.transitionPending(id, 'dismissed') } @@ -75,6 +79,14 @@ export class CrashReportStore { private async transitionPending( id: string, status: Exclude + ): Promise { + return this.transitionStatus(id, 'pending', status) + } + + private async transitionStatus( + id: string, + from: CrashReportStatus, + status: Exclude ): Promise { return this.withWrite(async (reports) => { let result: CrashReportRecord | null = null @@ -82,7 +94,7 @@ export class CrashReportStore { if (report.id !== id) { return report } - if (report.status !== 'pending') { + if (report.status !== from) { result = report return report } diff --git a/src/main/ipc/crash-reporting.test.ts b/src/main/ipc/crash-reporting.test.ts index c89012e87de..7166d5238f4 100644 --- a/src/main/ipc/crash-reporting.test.ts +++ b/src/main/ipc/crash-reporting.test.ts @@ -76,6 +76,34 @@ describe('registerCrashReportingHandlers', () => { ) }) + it('submits a dismissed report when the already-open prompt sends it', async () => { + const dismissed = report('dismissed', 'crash-already-dismissed') + const sent = report('sent', dismissed.id) + const markDismissedSent = vi.fn(async () => sent) + registerCrashReportingHandlers({ + getLatestPending: vi.fn(async () => null), + getById: vi.fn(async () => dismissed), + dismiss: vi.fn(), + markSent: vi.fn(), + markDismissedSent, + listRecent: vi.fn(async () => []), + record: vi.fn(), + formatDiagnosticText: vi.fn() + } as never) + + const result = await handlers.get('crashReports:submit')?.(null, { + reportId: dismissed.id, + githubLogin: null, + githubEmail: null + }) + + expect(result).toEqual({ ok: true, report: sent }) + expect(submitFeedbackMock).toHaveBeenCalledWith( + expect.objectContaining({ feedback: expect.stringContaining('[Crash Report]') }) + ) + expect(markDismissedSent).toHaveBeenCalledWith(dismissed.id) + }) + it('submits through feedback and marks the report sent only after success', async () => { const latest = report('pending', 'crash-submit-success') const sent = report('sent', latest.id) diff --git a/src/main/ipc/crash-reporting.ts b/src/main/ipc/crash-reporting.ts index 7fd022afbc7..16041ead2b0 100644 --- a/src/main/ipc/crash-reporting.ts +++ b/src/main/ipc/crash-reporting.ts @@ -61,7 +61,11 @@ export function registerCrashReportingHandlers(store: CrashReportStore): void { if (!report) { return { ok: false, status: null, error: 'No crash report available.' } } - if (report.status !== 'pending' || uploadedReportIds.has(report.id)) { + const canSubmitDismissedReport = Boolean(args.reportId && report.status === 'dismissed') + if ( + (!canSubmitDismissedReport && report.status !== 'pending') || + uploadedReportIds.has(report.id) + ) { return { ok: true, report: uploadedReportIds.has(report.id) ? { ...report, status: 'sent' } : report @@ -88,6 +92,17 @@ export function registerCrashReportingHandlers(store: CrashReportStore): void { return { ...result, report } } uploadedReportIds.add(report.id) + if (report.status === 'dismissed') { + try { + // Why: startup prompts are dismissed before the user can send from + // the still-open dialog, so successful uploads must update storage. + const sent = await store.markDismissedSent(report.id) + return { ok: true, report: sent ?? { ...report, status: 'sent' } } + } catch (error) { + console.error('[crash-reporting] Failed to mark dismissed crash report sent:', error) + return { ok: true, report: { ...report, status: 'sent' } } + } + } try { const sent = await store.markSent(report.id) return { ok: true, report: sent ?? { ...report, status: 'sent' } } diff --git a/src/renderer/src/components/crash-report/CrashReportDialog.tsx b/src/renderer/src/components/crash-report/CrashReportDialog.tsx index 9e188850bc8..dac92d4a040 100644 --- a/src/renderer/src/components/crash-report/CrashReportDialog.tsx +++ b/src/renderer/src/components/crash-report/CrashReportDialog.tsx @@ -34,7 +34,18 @@ export function CrashReportDialog(): React.JSX.Element { setLoading(true) try { const pending = await window.api.crashReports.getLatestPending() - setReport(pending) + let displayedReport = pending + if (pending && promptIfPresent) { + try { + // Why: startup crash prompts are one-shot. The open dialog keeps the + // report data locally if the user chooses to send immediately. + await window.api.crashReports.dismiss({ reportId: pending.id }) + displayedReport = { ...pending, status: 'dismissed' as const } + } catch (error) { + console.error('Failed to dismiss crash report after startup prompt:', error) + } + } + setReport(displayedReport) if (pending && promptIfPresent) { setOpen(true) } @@ -70,10 +81,15 @@ export function CrashReportDialog(): React.JSX.Element { toast.success('Crash report copied.') } - const handleDismiss = async (): Promise => { + const dismissReportIfNeeded = async (): Promise => { if (report?.status === 'pending') { await window.api.crashReports.dismiss({ reportId: report.id }) + setReport({ ...report, status: 'dismissed' }) } + } + + const handleDismiss = async (): Promise => { + await dismissReportIfNeeded() setOpen(false) } @@ -112,7 +128,11 @@ export function CrashReportDialog(): React.JSX.Element { if (submitting && !nextOpen) { return } - setOpen(nextOpen) + if (!nextOpen) { + void dismissReportIfNeeded().finally(() => setOpen(false)) + return + } + setOpen(true) }} >