Skip to content

fix(newsletter): modal re-pops when dismissal can't be persisted (silent SQLite write failure) #2321

Description

@peppescg

Summary

The newsletter signup modal re-pops repeatedly and won't stay dismissed for some users. The root cause is that the dismissal is never persisted because the SQLite write fails silently. Annoyed users then submit junk emails to make the modal go away — which is the source of the "newsletter spam" the marketing team has been triaging (see #2312, closed: it's not bots).

How it manifests (Sentry evidence)

From renderer analytics (stacklok/toolhive-studio, events are span.op:user.event):

  • Newsletter dismissed events vastly outnumber Newsletter modal shown (e.g. on release 0.36.2, dismissed/shown ≈ 2.7× on macOS), which is only possible if the modal keeps reappearing.
  • Heavy tail concentrated on release 0.34.0: ~430 dismissals / 35 installs in 7d (≈12/install/week) vs ~1.0/install on all current releases (0.35.x / 0.36.x are healthy).
  • One install was observed dismissing the modal 9 times within ~5 minutes in a single session.
  • These are real app installs (renderer spans), not API bots — a bot POSTing to api.hsforms.com never runs the renderer.

Caveat: custom.user_id (= thv instance_id) is not a perfectly unique per-machine key, so absolute per-user counts are soft. The mechanism below, however, is verified at the code level.

Direct cause

The dismissal is not saved: the SQLite write fails silently.

  • setNewsletterDismissedAtwriteSetting either returns early (read-only DB) or throws (caught and only logged locally). Either way newsletterDismissedAt is never stored, so on the next launch shouldShowAfterDismissal() returns true and the modal shows again.
  • Amplifier (renderer): in newsletter-modal.tsx, onDismiss calls queryClient.invalidateQueries(['newsletter-state']) but does not set the in-session closed flag (only onClose does). When the write fails, the refetch still returns dismissedAt = '' → the modal immediately reappears in the same session → dismiss → reappear → loop. This is the "9 dismisses in 5 minutes."

Why the write fails

Bucket A — DB is read-only (confirmed).
migrator.ts calls setDbWritable(false) when the on-disk schema is newer than the app knows (highestApplied > highestKnown). After that, every writeSetting is a silent no-op (if (!isDbWritable()) return). This happens when an older app runs against a DB migrated by a newer version — e.g. a newer build ran once on the same machine (on macOS all versions share ~/Library/Application Support/ToolHive), a downgrade, a restored backup, or a beta/RC then back to an older stable. Matches the 0.34.0 concentration (0.34.0 knows migrations up to v6; migration 007 shipped in 0.35.0 → only ≥0.35 bumps the DB to v7).
Note: migration 007 is purely additive (ALTER TABLE threads ADD COLUMN), so the blanket read-only block is arguably overly conservative.

Bucket B — the SQLite write genuinely throws (not yet measured).
dbWritable is true but db.prepare().run() throws and the try/catch swallows it. Possible causes: WAL on a network/synced filesystem (OneDrive/SMB/NFS → SQLITE_IOERR), disk full (SQLITE_FULL), permissions (EACCES/EPERM), locked file (SQLITE_BUSY), read-only mount (e.g. running from a DMG), corruption (SQLITE_CORRUPT), or the native module failing to load (we do see GLIBC_2.33 not found for better-sqlite3 on old Linux, ~10/30d). None of these are observable today — they only go to local electron-log, never Sentry. The read-only Bucket A path doesn't even produce a local log (silent early return).

Observability gap

There is no telemetry for write failures: logger.ts has no Sentry integration and sentry.ts has no console capture. The if (!isDbWritable()) return branch is completely silent. We literally cannot answer "how often / on which OS" until we instrument it.

Proposed work (suggest splitting into PRs)

PR1 — stop the symptom + add observability (small, low risk)

  • In newsletter-modal.tsx, set the in-session closed flag on dismiss so the modal can't re-pop within a session even if the write fails.
  • Don't show the modal when the DB is not writable (surface isDbWritable() to the renderer via getNewsletterState).
  • Instrument write failures with Sentry.captureException, separating Bucket A (skipped because !isDbWritable()) from Bucket B (write threw), with os / release / appliedSchema vs knownSchema.

PR2 — fix the root read-only behavior (separate, needs review/tests)

  • Relax setDbWritable(false) so additive-only migration gaps don't make the whole DB read-only (e.g. classify migrations additive vs breaking, block only on breaking), or at least keep tables the old app knows writable. If we keep the block, surface it to the user ("you're running an older version against newer data — please update") instead of degrading silently.

PR3 — Bucket B mitigations (data-driven, after PR1 ships)

  • Based on which SQLITE_* codes show up: e.g. WAL→DELETE journal fallback, busy_timeout pragma, and/or a renderer-side localStorage fallback for the dismissal.

Acceptance criteria (PR1)

  • Dismissing the newsletter modal never causes it to reappear within the same session, even when the dismissal can't be persisted (read-only DB or failed write).
  • DB write failures (both buckets) are visible in Sentry with OS / release / schema context.

Scope note: PR1 intentionally does not fully suppress the modal on read-only DBs. We keep showing it (so users can still subscribe) and only stop the in-session re-pop loop, while we ship telemetry first and measure how prevalent the read-only case is. Full read-only suppression is deferred to a follow-up PR, driven by that data.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageIssue needs initial triage by a maintainer

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions