Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion main/src/db/migrator.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
}
Expand Down
43 changes: 43 additions & 0 deletions main/src/db/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,46 @@ 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,
})
// 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)
})
}
19 changes: 12 additions & 7 deletions main/src/db/writers/settings-writer.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
9 changes: 8 additions & 1 deletion main/src/newsletter.ts
Original file line number Diff line number Diff line change
@@ -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'

Expand Down Expand Up @@ -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
}
}
44 changes: 33 additions & 11 deletions main/src/tests/newsletter.test.ts
Original file line number Diff line number Diff line change
@@ -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) =>
Expand All @@ -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() }
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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)
})
})
})
4 changes: 2 additions & 2 deletions preload/src/api/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<{
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ describe('NewsletterModal', () => {
.mockResolvedValue(undefined)
window.electronAPI.setNewsletterDismissedAt = vi
.fn()
.mockResolvedValue(undefined)
.mockResolvedValue(true)

server.use(
http.post(HUBSPOT_URL, () =>
Expand Down Expand Up @@ -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()
})
})
})
14 changes: 10 additions & 4 deletions renderer/src/common/components/newsletter-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ function NewsletterDialog({
}: {
successMessage: string
onSubscribed: (message: string) => void
onDismiss: () => void
onDismiss: (persisted: boolean) => void
onClose: () => void
}) {
const [email, setEmail] = useState('')
Expand Down Expand Up @@ -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)
},
})

Expand Down Expand Up @@ -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)
Comment thread
peppescg marked this conversation as resolved.
closeNewsletterModal()
queryClient.invalidateQueries({ queryKey: ['newsletter-state'] })
}}
Expand Down
2 changes: 1 addition & 1 deletion renderer/src/common/mocks/electronAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: '' }),
Expand Down
Loading