Skip to content

fix(desktop): fence localStorage SecurityError from killing the React tree - #5142

Merged
wesbillman merged 3 commits into
block:mainfrom
iroiro147:fix/5078-error-boundary-storage
Aug 7, 2026
Merged

fix(desktop): fence localStorage SecurityError from killing the React tree#5142
wesbillman merged 3 commits into
block:mainfrom
iroiro147:fix/5078-error-boundary-storage

Conversation

@iroiro147

Copy link
Copy Markdown
Contributor

Summary

WebKit throws SecurityError from localStorage.getItem (not just setItem) when storage access is denied for the origin. With no ErrorBoundary in desktop/src, any such throw inside a provider render (ThemeProvider, CommunitiesProvider, App boot) propagated to the reconciler, unmounted the root, and left a blank window. Measured repro in #5078: a single throwing getItem on buzz-communities or buzz-active-community-id kills the container.

Closes #5078.

What changed

New helper — desktop/src/shared/lib/safeStorage.ts

  • getStorageItem(key, fallback?) — wraps window.localStorage.getItem; on a thrown error (SecurityError under denied-storage origin) it warns once per key and returns the fallback.
  • setStorageItem(key, value) and removeStorageItem(key) — same fail-closed contract (return false on throw).
  • Unit tests in safeStorage.test.mjs cover the happy path and the SecurityError path.

Rewired the init-path readers that ran before any UI existed

  • desktop/src/features/communities/communityStorage.tsmigrateLegacyCommunityStorage, loadCommunities, loadActiveCommunityId, loadCommunityDiscoveryAfterLeave, initFirstCommunity
  • desktop/src/features/communities/legacyCommunityStorage.tsmigrateLegacyCommunityStorageBeforeRender
  • desktop/src/shared/theme/ThemeProvider.tsxreadStoredTheme, applyCachedVars, the useState initialisers for accentColor and followSystem, and the accent re-read inside applyTheme

Root-level fence — desktop/src/app/RootErrorBoundary.tsx

  • New top-level ErrorBoundary wrapping the whole provider tree in main.tsx. Any remaining uncaught render error (a future storage read that bypasses the helper, or any other render-time crash) renders a degraded splash with a Reload button instead of a blank window.

Test plan

  • desktop/src/shared/lib/safeStorage.test.mjs — node --test runner, 11 assertions across healthy, absent, and SecurityError-throwing storage.
  • Full just ci runs on the blocker.
  • Existing communityStorage.test.mjs and legacyCommunityStorage.test.mjs continue to pass (they exercise the same functions via in-memory Storage doubles; the new code path in migrateLegacyCommunityStorage only adds a try/catch around the same body).

Why not an ErrorBoundary-only fix

A boundary alone can't help on a clean mount — the first throw already unmounted the whole subtree before any state or fallback data was loaded, so retrying would hit the same throw on the very next render. The storage accessor has to fail closed and the boundary has to exist for whatever bypasses it. Both are needed; neither is sufficient alone.

… tree

WebKit throws SecurityError from localStorage.getItem (not just setItem)
when storage access is denied for the origin. With no ErrorBoundary in
desktop/src, any such throw inside a provider render (ThemeProvider,
CommunitiesProvider, App boot) unmounted the whole root and left a blank
window. Measured in the repro arms attached to block#5078: a single throwing
getItem on 'buzz-communities' or 'buzz-active-community-id' kills the
container.

Adds shared/lib/safeStorage: getStorageItem / setStorageItem /
removeStorageItem which fail closed (null / false) and warn-once per key
instead of propagating. Rewires the init-path readers that ran before any
UI existed:

  - communityStorage.ts: migrateLegacyCommunityStorage, loadCommunities,
    loadActiveCommunityId, loadCommunityDiscoveryAfterLeave,
    initFirstCommunity
  - legacyCommunityStorage.ts: migrateLegacyCommunityStorageBeforeRender
  - ThemeProvider.tsx: readStoredTheme, applyCachedVars, useState
    initializers for accentColor and followSystem
  - ThemeProvider.applyTheme: accent re-read

Also installs a root-level RootErrorBoundary in main.tsx so any remaining
uncaught render error (any future storage read that bypasses the helper)
renders a degraded splash with a Reload affordance instead of a blank
window. Includes unit tests for the safeStorage helpers
(safeStorage.test.mjs) covering the happy path and the SecurityError path.

Refs block#5078

Signed-off-by: iroiro147 <sarthak.singh@mastersunion.org>
@iroiro147
iroiro147 requested a review from a team as a code owner August 7, 2026 03:01

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl reviewing on behalf of Wes at exact head 4d832212147d44c3b77cddf4b15333ae1261f4d6 as a release/security gate.

The production approach is directionally sound: the diff is confined to the seven advertised desktop files; I found no dependency, workflow, native-command, network, credential, or code-loading changes; the guarded startup reads preserve healthy-storage values and fail closed under denied storage. I am requesting changes before release for the following blockers:

  1. The newly added tests do not run in the repository's actual test environment. pnpm test fails all six safeStorage.test.mjs cases with ReferenceError: window is not defined at patchLocalStorage (safeStorage.test.mjs:31). Result: 4480 pass / 6 fail. These tests need to install/restore a globalThis.window (using the repo's established DOM-test pattern) rather than assuming the Node runner supplies one.

  2. The acceptance behavior is not tested end to end. The helper tests—even once repaired—only call three wrappers directly. They do not prove that a denied localStorage.getItem during CommunitiesProvider/ThemeProvider initialization leaves a visible tree, nor that the root boundary renders a recoverable surface for a remaining render-time throw. Please add a deterministic mounted regression that injects a throwing storage implementation before mount and asserts a non-empty, actionable fallback/default UI. This is the core release guarantee, not decorative shrubbery.

  3. The root fallback renders arbitrary error.message text to the user (RootErrorBoundary.tsx:40-42). Because this boundary catches every render error, those messages can contain internal paths, request details, identifiers, or other diagnostic data. Keep the UI generic and log the diagnostic only via componentDidCatch.

Additionally, pnpm check reports a formatter error introduced in main.tsx:84-99; the other reported lint items pre-exist outside this diff. pnpm typecheck and git diff --check pass.

I also traced bootstrap before the boundary: the production storage paths currently invoked there (recoverLocalStorageQuotaOnStartup and legacy migration/application) are locally caught, so I did not find a separate pre-boundary production escape in this diff. Fix the failing tests, prove the actual mounted denial/fallback behavior, avoid exposing raw errors, and format the file; then this should be straightforward to reassess.

wesbillman and others added 2 commits August 7, 2026 08:21
Make the safe-storage tests run in the repository's Node harness, exercise
the denied-storage provider path and root fallback as mounted React trees,
and keep arbitrary exception details out of the user-visible recovery UI.
Format the provider tree so the change passes the repository checks.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman
wesbillman enabled auto-merge (squash) August 7, 2026 14:39
@wesbillman
wesbillman disabled auto-merge August 7, 2026 16:36
@wesbillman
wesbillman merged commit 8630e58 into block:main Aug 7, 2026
26 checks passed
atishpatel added a commit that referenced this pull request Aug 7, 2026
…-log-harness

* origin/main:
  feat(desktop): adding rich link previews to messages (#3818)
  fix(buzz-agent): Responses reasoning summary, Anthropic display:summarized, ACP v2 messageId (#5195)
  fix(desktop): retain distinct agent instances in autocomplete (#5202)
  fix(desktop): defer channel visibility change to Save (#5203)
  feat(desktop): Projects follow-ups — access restrictions, fast loading, activity feed polish (#5073)
  refactor(cli): replace probe/decider/detail split with single typed extractor (#5191)
  fix(desktop): drop unhandled rejection from throwing window.Notification (#5143)
  fix(desktop): fence localStorage SecurityError from killing the React tree (#5142)
  fix(desktop): make terminal output selectable (#4980)
  fix(desktop): use WEBKIT_DMABUF_RENDERER_FORCE_SHM for NVIDIA/AppImage (#3654) (#4505)
  Make public starter channels best effort (#5192)
  Mobile: add anchored reaction popover (#5025)
  feat(mobile): add bee pull-to-refresh (#5059)

Signed-off-by: Atish Patel <atish@squareup.com>
wpfleger96 pushed a commit that referenced this pull request Aug 7, 2026
* origin/main: (32 commits)
  Recover from max-token response truncation (#5223)
  chore(release): release Buzz Desktop version 0.5.6 (#5214)
  fix(mobile): keep latest messages above composer (#4981)
  fix(sdk): preserve self-mention p tags in message and forum event builders (#4975)
  bump @tauri-apps/cli to ~2.11.4 to fix linux app icon issue (#4858)
  feat(desktop): adding rich link previews to messages (#3818)
  fix(buzz-agent): Responses reasoning summary, Anthropic display:summarized, ACP v2 messageId (#5195)
  fix(desktop): retain distinct agent instances in autocomplete (#5202)
  fix(desktop): defer channel visibility change to Save (#5203)
  feat(desktop): Projects follow-ups — access restrictions, fast loading, activity feed polish (#5073)
  refactor(cli): replace probe/decider/detail split with single typed extractor (#5191)
  fix(desktop): drop unhandled rejection from throwing window.Notification (#5143)
  fix(desktop): fence localStorage SecurityError from killing the React tree (#5142)
  fix(desktop): make terminal output selectable (#4980)
  fix(desktop): use WEBKIT_DMABUF_RENDERER_FORCE_SHM for NVIDIA/AppImage (#3654) (#4505)
  Make public starter channels best effort (#5192)
  Mobile: add anchored reaction popover (#5025)
  feat(mobile): add bee pull-to-refresh (#5059)
  Remove agent creation success modal (#5063)
  fix(buzz-agent): escalate LLM timeouts per retry and log per-call latency (#5130)
  ...

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

# Conflicts:
#	desktop/src/shared/api/tauri.ts
wpfleger96 pushed a commit that referenced this pull request Aug 7, 2026
…format

* origin/main: (60 commits)
  feat(desktop): unify add agent flows (#5015)
  fix(buzz-agent): budget summarizer reasoning separately so it cannot starve the handoff summary (#5248)
  infra: bind development services to loopback (#4871)
  chore(release): release Buzz Desktop version 0.5.7 (#5252)
  fix(desktop): isolate relay admission tests (#5221)
  fix(desktop): externalize boot <style> to prevent Tauri CSP nonce override (#5242)
  fix(desktop): let imported and recovered identities finish onboarding (#5228)
  Recover from max-token response truncation (#5223)
  chore(release): release Buzz Desktop version 0.5.6 (#5214)
  fix(mobile): keep latest messages above composer (#4981)
  fix(sdk): preserve self-mention p tags in message and forum event builders (#4975)
  bump @tauri-apps/cli to ~2.11.4 to fix linux app icon issue (#4858)
  feat(desktop): adding rich link previews to messages (#3818)
  fix(buzz-agent): Responses reasoning summary, Anthropic display:summarized, ACP v2 messageId (#5195)
  fix(desktop): retain distinct agent instances in autocomplete (#5202)
  fix(desktop): defer channel visibility change to Save (#5203)
  feat(desktop): Projects follow-ups — access restrictions, fast loading, activity feed polish (#5073)
  refactor(cli): replace probe/decider/detail split with single typed extractor (#5191)
  fix(desktop): drop unhandled rejection from throwing window.Notification (#5143)
  fix(desktop): fence localStorage SecurityError from killing the React tree (#5142)
  ...

Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A throwing localStorage.getItem kills the whole desktop tree (no ErrorBoundary anywhere in desktop/src)

2 participants