fix(newsletter): stop dismissal re-pop loop and add DB write observability

The newsletter modal re-pops because the dismissal is never persisted when the
SQLite write fails silently. First step of #2321:

- setNewsletterDismissedAt now reports whether the dismissal was actually
  persisted. The renderer suppresses the modal for the rest of the session only
  when it wasn't — stopping the in-session re-pop loop on a read-only DB or a
  failed write, while leaving the normal (persisted) flow unchanged.
- Instrument DB write failures in Sentry, separating Bucket A (read-only DB:
  on-disk schema newer than the app, captured in the migrator with applied vs
  known schema) from Bucket B (the write threw, captured with the SQLite error
  code). os.name / release are attached automatically. Today these failures are
  invisible — Bucket A doesn't even produce a local log.

Refs #2321

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: peppescg

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
       }

main/src/db/telemetry.ts

@@ -32,3 +32,44 @@ export function withDbSpan<T>(
     }
   )
 }
+
+// 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,
+    })
+    Sentry.captureMessage(
+      `[DB] Read-only: on-disk schema v${appliedSchema} is newer than app v${knownSchema}; 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)
+  })
+}

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 {

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
   }
 }

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)
     })
   })
 })

preload/src/api/app.ts

@@ -31,7 +31,7 @@ export const appApi = {
   }> => ipcRenderer.invoke('get-newsletter-state'),
   setNewsletterSubscribed: (subscribed: boolean): Promise<void> =>
     ipcRenderer.invoke('set-newsletter-subscribed', subscribed),
-  setNewsletterDismissedAt: (dismissedAt: string): Promise<void> =>
+  setNewsletterDismissedAt: (dismissedAt: string): Promise<boolean> =>
     ipcRenderer.invoke('set-newsletter-dismissed-at', dismissedAt),
 
   getExpertConsultationState: (): Promise<{
@@ -66,7 +66,7 @@ export interface AppAPI {
     dismissedAt: string
   }>
   setNewsletterSubscribed: (subscribed: boolean) => Promise<void>
-  setNewsletterDismissedAt: (dismissedAt: string) => Promise<void>
+  setNewsletterDismissedAt: (dismissedAt: string) => Promise<boolean>
   getExpertConsultationState: () => Promise<{
     submitted: boolean
     dismissedAt: string

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(<NewsletterModal />)
+
+      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()
+    })
   })
 })

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() {
     <NewsletterDialog
       successMessage={successMessage}
       onSubscribed={(message) => 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'] })
       }}

renderer/src/common/mocks/electronAPI.ts

@@ -15,7 +15,7 @@ function createElectronStub(): Partial<ElectronAPI> {
       .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: '' }),