diff --git a/main/src/db/migrator.ts b/main/src/db/migrator.ts index 40679139c..331410708 100644 --- a/main/src/db/migrator.ts +++ b/main/src/db/migrator.ts @@ -1,7 +1,7 @@ import type Database from 'better-sqlite3' import { getDb, setDbWritable } from './database' import log from '../logger' -import { withDbSpan } from './telemetry' +import { withDbSpan, captureDbReadOnly } from './telemetry' interface Migration { id: number @@ -58,6 +58,7 @@ export function runMigrations(): void { `[DB] Database schema (v${highestApplied}) is newer than app (v${highestKnown}). ` + 'Skipping SQLite writes to avoid corruption.' ) + captureDbReadOnly(highestApplied, highestKnown) setDbWritable(false) return } diff --git a/main/src/db/telemetry.ts b/main/src/db/telemetry.ts index d4f9574b5..9c563f96d 100644 --- a/main/src/db/telemetry.ts +++ b/main/src/db/telemetry.ts @@ -32,3 +32,46 @@ export function withDbSpan( } ) } + +// Bucket A: the on-disk schema is newer than the running app, so the migrator +// disabled all writes (see migrator.ts). Reported once per launch on affected +// installs. os.name / release are attached automatically by @sentry/electron. +export function captureDbReadOnly( + appliedSchema: number, + knownSchema: number +): void { + Sentry.withScope((scope) => { + scope.setTag('db.failure_bucket', 'schema_newer') + scope.setExtras({ + 'db.applied_schema': appliedSchema, + 'db.known_schema': knownSchema, + }) + // Stable message so Sentry groups all occurrences into one issue; the + // schema versions live in extras above rather than in the message. + Sentry.captureMessage( + '[DB] Read-only: on-disk schema is newer than app; SQLite writes disabled', + 'warning' + ) + }) +} + +// Bucket B: a SQLite write actually threw (filesystem/permission/lock/disk/ +// corruption). The native `code` (e.g. SQLITE_IOERR, SQLITE_FULL, SQLITE_BUSY) +// is surfaced as a tag so we can tell the failure modes apart. +export function captureDbWriteFailure( + table: string, + key: string, + error: unknown +): void { + const code = + typeof error === 'object' && error !== null && 'code' in error + ? String((error as { code: unknown }).code) + : undefined + Sentry.withScope((scope) => { + scope.setTag('db.failure_bucket', 'write_threw') + scope.setTag('db.table', table) + if (code) scope.setTag('db.sqlite_code', code) + scope.setExtras({ 'db.key': key }) + Sentry.captureException(error) + }) +} diff --git a/main/src/db/writers/settings-writer.ts b/main/src/db/writers/settings-writer.ts index e6c193f93..c1e85997b 100644 --- a/main/src/db/writers/settings-writer.ts +++ b/main/src/db/writers/settings-writer.ts @@ -1,14 +1,19 @@ import { getDb, isDbWritable } from '../database' -import { withDbSpan } from '../telemetry' +import { withDbSpan, captureDbWriteFailure } from '../telemetry' export function writeSetting(key: string, value: string): void { if (!isDbWritable()) return - withDbSpan('DB write setting', 'db.write', { 'db.key': key }, () => { - const db = getDb() - db.prepare( - 'INSERT OR REPLACE INTO settings (key, value) VALUES (?, ?)' - ).run(key, value) - }) + try { + withDbSpan('DB write setting', 'db.write', { 'db.key': key }, () => { + const db = getDb() + db.prepare( + 'INSERT OR REPLACE INTO settings (key, value) VALUES (?, ?)' + ).run(key, value) + }) + } catch (err) { + captureDbWriteFailure('settings', key, err) + throw err + } } export function deleteSetting(key: string): void { diff --git a/main/src/newsletter.ts b/main/src/newsletter.ts index da9eaf57d..12a8e637b 100644 --- a/main/src/newsletter.ts +++ b/main/src/newsletter.ts @@ -1,5 +1,6 @@ import Store from 'electron-store' import log from './logger' +import { isDbWritable } from './db/database' import { writeSetting } from './db/writers/settings-writer' import { readSetting } from './db/readers/settings-reader' @@ -50,10 +51,16 @@ export function setNewsletterSubscribed(subscribed: boolean): void { } } -export function setNewsletterDismissedAt(dismissedAt: string): void { +// Returns whether the dismissal was actually persisted. `false` means the +// write was skipped (read-only DB — Bucket A) or threw (Bucket B); the renderer +// uses this to suppress the modal for the session so it doesn't loop. +export function setNewsletterDismissedAt(dismissedAt: string): boolean { + if (!isDbWritable()) return false try { writeSetting('newsletterDismissedAt', dismissedAt) + return true } catch (err) { log.error('[DB] Failed to write newsletterDismissedAt:', err) + return false } } diff --git a/main/src/tests/newsletter.test.ts b/main/src/tests/newsletter.test.ts index 70ec8a60a..e6dcd2a96 100644 --- a/main/src/tests/newsletter.test.ts +++ b/main/src/tests/newsletter.test.ts @@ -1,9 +1,12 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' -const { mockReadSetting, mockWriteSetting } = vi.hoisted(() => ({ - mockReadSetting: vi.fn(), - mockWriteSetting: vi.fn(), -})) +const { mockReadSetting, mockWriteSetting, mockIsDbWritable } = vi.hoisted( + () => ({ + mockReadSetting: vi.fn(), + mockWriteSetting: vi.fn(), + mockIsDbWritable: vi.fn(() => true), + }) +) vi.mock('@sentry/electron/main', () => ({ startSpan: vi.fn((_opts: unknown, cb: (span: unknown) => unknown) => @@ -28,6 +31,10 @@ vi.mock('../db/readers/settings-reader', () => ({ readSetting: mockReadSetting, })) +vi.mock('../db/database', () => ({ + isDbWritable: mockIsDbWritable, +})) + vi.mock('electron-store', () => ({ default: vi.fn(function ElectronStore() { return { get: vi.fn(), set: vi.fn() } @@ -42,7 +49,10 @@ import { describe('newsletter', () => { beforeEach(() => { - vi.clearAllMocks() + // resetAllMocks (not just clearAllMocks) so a throwing writeSetting/readSetting + // implementation set by one test doesn't leak into the next. + vi.resetAllMocks() + mockIsDbWritable.mockReturnValue(true) }) describe('getNewsletterState', () => { @@ -100,24 +110,36 @@ describe('newsletter', () => { }) describe('setNewsletterDismissedAt', () => { - it('writes to SQLite', () => { + it('writes to SQLite and returns true on success', () => { const timestamp = '2026-03-18T10:00:00.000Z' - setNewsletterDismissedAt(timestamp) + const persisted = setNewsletterDismissedAt(timestamp) expect(mockWriteSetting).toHaveBeenCalledWith( 'newsletterDismissedAt', timestamp ) + expect(persisted).toBe(true) }) - it('does not throw when SQLite write fails', () => { + it('returns false and skips the write when the DB is read-only', () => { + mockIsDbWritable.mockReturnValue(false) + + const persisted = setNewsletterDismissedAt('2026-03-18T10:00:00.000Z') + + expect(persisted).toBe(false) + expect(mockWriteSetting).not.toHaveBeenCalled() + }) + + it('returns false (and does not throw) when the SQLite write fails', () => { mockWriteSetting.mockImplementation(() => { throw new Error('DB write failed') }) - expect(() => - setNewsletterDismissedAt('2026-03-18T10:00:00.000Z') - ).not.toThrow() + let persisted: boolean | undefined + expect(() => { + persisted = setNewsletterDismissedAt('2026-03-18T10:00:00.000Z') + }).not.toThrow() + expect(persisted).toBe(false) }) }) }) diff --git a/preload/src/api/app.ts b/preload/src/api/app.ts index 5df892f50..53e9f7ced 100644 --- a/preload/src/api/app.ts +++ b/preload/src/api/app.ts @@ -31,7 +31,7 @@ export const appApi = { }> => ipcRenderer.invoke('get-newsletter-state'), setNewsletterSubscribed: (subscribed: boolean): Promise => ipcRenderer.invoke('set-newsletter-subscribed', subscribed), - setNewsletterDismissedAt: (dismissedAt: string): Promise => + setNewsletterDismissedAt: (dismissedAt: string): Promise => ipcRenderer.invoke('set-newsletter-dismissed-at', dismissedAt), getExpertConsultationState: (): Promise<{ @@ -66,7 +66,7 @@ export interface AppAPI { dismissedAt: string }> setNewsletterSubscribed: (subscribed: boolean) => Promise - setNewsletterDismissedAt: (dismissedAt: string) => Promise + setNewsletterDismissedAt: (dismissedAt: string) => Promise getExpertConsultationState: () => Promise<{ submitted: boolean dismissedAt: string diff --git a/renderer/src/common/components/__tests__/newsletter-modal.test.tsx b/renderer/src/common/components/__tests__/newsletter-modal.test.tsx index 9ce73ff72..0eb6653ef 100644 --- a/renderer/src/common/components/__tests__/newsletter-modal.test.tsx +++ b/renderer/src/common/components/__tests__/newsletter-modal.test.tsx @@ -60,7 +60,7 @@ describe('NewsletterModal', () => { .mockResolvedValue(undefined) window.electronAPI.setNewsletterDismissedAt = vi .fn() - .mockResolvedValue(undefined) + .mockResolvedValue(true) server.use( http.post(HUBSPOT_URL, () => @@ -294,5 +294,38 @@ describe('NewsletterModal', () => { .mock.calls[0]?.[0] expect(new Date(calledWith!).getTime()).not.toBeNaN() }) + + it('does not re-show the modal in the same session when the dismissal cannot be persisted', async () => { + const title = `Stay up to date with improvements to ${APP_DISPLAY_NAME}` + // Read-only DB: the write never persists, so getNewsletterState keeps + // returning an empty dismissedAt across refetches. + window.electronAPI.setNewsletterDismissedAt = vi + .fn() + .mockResolvedValue(false) + window.electronAPI.getNewsletterState = vi + .fn() + .mockResolvedValue({ subscribed: false, dismissedAt: '' }) + + renderWithProviders() + + await waitFor(() => { + expect(screen.getByText(title)).toBeVisible() + }) + + const initialStateCalls = vi.mocked(window.electronAPI.getNewsletterState) + .mock.calls.length + + await userEvent.click(screen.getByRole('button', { name: /close/i })) + + // The dismiss triggers an invalidate -> refetch of the newsletter state. + await waitFor(() => { + expect( + vi.mocked(window.electronAPI.getNewsletterState).mock.calls.length + ).toBeGreaterThan(initialStateCalls) + }) + + // Even though the dismissal wasn't persisted, the modal must stay gone. + expect(screen.queryByText(title)).not.toBeInTheDocument() + }) }) }) diff --git a/renderer/src/common/components/newsletter-modal.tsx b/renderer/src/common/components/newsletter-modal.tsx index bfca40a66..e859623c8 100644 --- a/renderer/src/common/components/newsletter-modal.tsx +++ b/renderer/src/common/components/newsletter-modal.tsx @@ -35,7 +35,7 @@ function NewsletterDialog({ }: { successMessage: string onSubscribed: (message: string) => void - onDismiss: () => void + onDismiss: (persisted: boolean) => void onClose: () => void }) { const [email, setEmail] = useState('') @@ -76,9 +76,9 @@ function NewsletterDialog({ const { mutate: dismiss } = useMutation({ mutationFn: () => window.electronAPI.setNewsletterDismissedAt(new Date().toISOString()), - onSuccess: () => { + onSuccess: (persisted) => { trackEvent('Newsletter dismissed') - onDismiss() + onDismiss(persisted) }, }) @@ -214,7 +214,13 @@ export function NewsletterModal() { setSuccessMessage(message)} - onDismiss={() => { + onDismiss={(persisted) => { + // Only when the dismissal could NOT be persisted (read-only DB — Bucket + // A — or a failed write — Bucket B): hide for the rest of the session, + // otherwise the invalidate->refetch below returns an empty dismissedAt + // and immediately re-shows the modal, looping within the session. + // When persistence works, the stored state handles suppression. + if (!persisted) setClosed(true) closeNewsletterModal() queryClient.invalidateQueries({ queryKey: ['newsletter-state'] }) }} diff --git a/renderer/src/common/mocks/electronAPI.ts b/renderer/src/common/mocks/electronAPI.ts index 0980a4750..a2446a4f4 100644 --- a/renderer/src/common/mocks/electronAPI.ts +++ b/renderer/src/common/mocks/electronAPI.ts @@ -15,7 +15,7 @@ function createElectronStub(): Partial { .fn() .mockResolvedValue({ subscribed: false, dismissedAt: '' }), setNewsletterSubscribed: vi.fn().mockResolvedValue(undefined), - setNewsletterDismissedAt: vi.fn().mockResolvedValue(undefined), + setNewsletterDismissedAt: vi.fn().mockResolvedValue(true), getExpertConsultationState: vi .fn() .mockResolvedValue({ submitted: false, dismissedAt: '' }),