Skip to content

[P1][bug] settingsStore swallows localStorage.setItem quota errors silently #454

Description

@EVWorth

Context

settingsStore persists QuerySettings and FormatterSettings to localStorage. If the write fails (quota exceeded), the failure is invisible.

Problem

Both setters wrap localStorage.setItem in a try/catch with an empty catch body:

// src/stores/settingsStore.ts:212-218
setQuerySettings: (settings) => {
  try {
    localStorage.setItem(QUERY_SETTINGS_KEY, JSON.stringify(settings));
  } catch {
    // localStorage unavailable
  }
  set({ querySettings: settings });
},

// src/stores/settingsStore.ts:219-225
setFormatterSettings: (settings) => {
  try {
    localStorage.setItem(STORAGE_KEY, JSON.stringify(settings));
  } catch {
    // localStorage unavailable
  }
  set({ formatterSettings: settings });
},

If the user is at the 5 MB localStorage quota (Chrome's hard cap), the setItem throws. The settings silently fall back to in-memory state — on next launch, the user's settings revert to defaults. There's no log, no toast, no degradation flag.

The themeStore.setTheme has the same pattern (src/stores/themeStore.ts:40-45) and is already flagged as #348.

The cross-cutting audit (docs/audits/cross-cutting.md F9) flagged this as P1.

Files

Repro

In DevTools console:

const _set = localStorage.setItem;
localStorage.setItem = () => { throw new Error("QuotaExceededError"); };
// Now click any setting — no error appears, no warning, settings "save" only in-memory.
localStorage.setItem = _set;

Expected

A console.warn (or tracing log) on every storage failure. A storageError: string | null field on the store. The StatusBar (or a toast) shows the error. The same fix should pair with #348 for themeStore.

Proposed fix

Scope S. Replace empty catches with:

} catch (e) {
  console.warn(`Failed to persist ${key}:`, e);
  set({ storageError: e instanceof Error ? e.message : String(e) });
}

Add storageError: string | null to the store interface. StatusBar (or a <StorageErrorToast /> portal) shows the message when non-null. Apply the same fix to themeStore.setTheme (paired with #348).

Acceptance

In the repro scenario, console shows the warning + a visible chip/toast. A test asserts the storageError field is set when setItem throws (use vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { throw new Error(...) })).

Needs human verify

Yes (localStorage quota not reproducible in CI without mock).

Labels: audit, area/cross-cutting, severity/p1, kind/bug

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/cross-cuttingAudit areaauditTracks a feature-by-feature codebase audit findingkind/bugAudit finding categoryseverity/p1Audit finding severity

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions