Guard the theme toggle's localStorage read so blocked storage can't crash it - #52
Merged
Merged
Conversation
…rash it The theme toggle reads the saved choice from localStorage on mount. The write was already wrapped in try/catch (with a comment) — but the read wasn't, and in a browser that blocks site storage (private mode, a sandboxed frame, "block all cookies") even *reading* localStorage throws, not just writing. That unguarded read sits in the mount effect with no error boundary above it, so the throw doesn't fail quietly: it tears down the whole client render, not just the theme. A storage-blocked visitor would get a broken app, and an uncaught error — the exact case the guards on the write and the pre-paint theme script were meant to cover, missed on the one path that reads first. Wrap the read in try/catch, falling back to System, so setMounted still runs and the toggle stays live for the session. A new e2e makes getItem throw and proves the toggle mounts, cycles, and applies a theme with no page error; verified it fails without the guard (the button doesn't even render — the render crashes). Fix mirrors the guarded write, the layout script, and the kit planner's storage handling. e2e 39 -> 40; no data or product-behaviour change beyond not crashing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The theme toggle reads the saved choice from
localStorageon mount. The write was already wrapped in try/catch (with a comment about disabled storage) — but the read wasn't. In a browser that blocks site storage (private mode, a sandboxed frame, "block all cookies"), even readinglocalStoragethrows, not just writing.That unguarded read sits in the mount effect with no error boundary above it, so the throw doesn't fail quietly — it tears down the whole client render, not just the theme. A storage-blocked visitor would get a broken app plus an uncaught error. It's the exact case the guards on the write and the pre-paint theme script were meant to cover, missed on the one path that reads first.
Found by auditing every
localStoragetouch for storage-blocked resilience — the kit planner (read + write) and the layout theme script are all guarded; this read was the only unguarded one.What
Wrap the read in try/catch, falling back to
System, sosetMountedstill runs and the toggle stays live for the session. Mirrors the guarded write, the layout script, and the kit planner's storage handling.Verification
smoke.spec.ts) makesgetItemthrow and proves the toggle mounts, cycles System → Light, applies the theme, and raises no page error.npm test197 · build · Playwright e2e 39 → 40.