Skip to content

fix(client): fall back to defaults when preset storage is corrupt - #5184

Open
jeffrey701 wants to merge 1 commit into
phase-rs:mainfrom
jeffrey701:fix/load-presets-corrupt-storage
Open

fix(client): fall back to defaults when preset storage is corrupt#5184
jeffrey701 wants to merge 1 commit into
phase-rs:mainfrom
jeffrey701:fix/load-presets-corrupt-storage

Conversation

@jeffrey701

Copy link
Copy Markdown
Contributor

Problem

loadPresets (client/src/services/presets.ts) is the only localStorage loader in the module that calls JSON.parse without a try/catch:

const saved = JSON.parse(raw) as GamePreset[];
return saved.length > 0 ? saved : DEFAULT_PRESETS;

Every sibling loader (loadActiveGame, loadWsSession, loadActiveQuickDraft, …) wraps the parse and returns a fallback. So a corrupt/partial phase-game-presets value (e.g. a write interrupted mid-flush, or an older schema) throws a SyntaxError. And because savePreset and deletePreset both call loadPresets() first, all three throw — the game-setup preset UI crashes rather than recovering.

Fix

Wrap the parse in try/catch and fall back to DEFAULT_PRESETS, matching the other loaders.

Tests

Adds presets.test.ts: corrupt storage ("{oops") makes loadPresets / savePreset / deletePreset throw before the fix and return/no-throw after; empty storage yields the defaults; valid storage round-trips a saved preset.

Self-contained: only presets.ts + a new test; no engine/Rust/protocol changes.

Model: claude-opus-4-8

@jeffrey701
jeffrey701 requested a review from matthewevans as a code owner July 6, 2026 05:24
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

loadPresets was the lone localStorage loader that JSON.parse'd without a try/catch (loadActiveGame, loadWsSession, loadActiveQuickDraft, … all guard and return a fallback). A malformed 'phase-game-presets' value threw a SyntaxError, and because savePreset/deletePreset both call loadPresets first, all three crashed and took down the game-setup preset UI. Wrap the parse and fall back to DEFAULT_PRESETS.
@jeffrey701
jeffrey701 force-pushed the fix/load-presets-corrupt-storage branch from 8a040ab to d059c30 Compare July 6, 2026 12:05
@matthewevans matthewevans added the defer-fe Frontend/client/UI PR deferred to Matt's direct review label Jul 6, 2026
@matthewevans

Copy link
Copy Markdown
Member

Deferred by maintainer intake policy — not ignored.

This current head (d059c301951e910370a2a2ba0f43ddbfbff37888) was triaged as a frontend-only change (client/src/services/__tests__/presets.test.ts, client/src/services/presets.ts) by jeffrey701. The local frontend-review allowlist does not include this author, so this route does not perform an implementation-diff review or approve the PR.

A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change.

3 similar comments
@matthewevans

Copy link
Copy Markdown
Member

Deferred by maintainer intake policy — not ignored.

This current head (d059c301951e910370a2a2ba0f43ddbfbff37888) was triaged as a frontend-only change (client/src/services/__tests__/presets.test.ts, client/src/services/presets.ts) by jeffrey701. The local frontend-review allowlist does not include this author, so this route does not perform an implementation-diff review or approve the PR.

A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change.

@matthewevans

Copy link
Copy Markdown
Member

Deferred by maintainer intake policy — not ignored.

This current head (d059c301951e910370a2a2ba0f43ddbfbff37888) was triaged as a frontend-only change (client/src/services/__tests__/presets.test.ts, client/src/services/presets.ts) by jeffrey701. The local frontend-review allowlist does not include this author, so this route does not perform an implementation-diff review or approve the PR.

A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change.

@matthewevans

Copy link
Copy Markdown
Member

Deferred by maintainer intake policy — not ignored.

This current head (d059c301951e910370a2a2ba0f43ddbfbff37888) was triaged as a frontend-only change (client/src/services/__tests__/presets.test.ts, client/src/services/presets.ts) by jeffrey701. The local frontend-review allowlist does not include this author, so this route does not perform an implementation-diff review or approve the PR.

A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change.

@matthewevans matthewevans self-assigned this Aug 1, 2026
@matthewevans matthewevans added the bug Bug fix label Aug 1, 2026
@matthewevans matthewevans removed their assignment Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix defer-fe Frontend/client/UI PR deferred to Matt's direct review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants