fix(ui): replace localStorage panel state restore with SvelteKit snapshot API to eliminate flash on load #115

New Issue

marcel · 2026-03-27T17:53:32+01:00

marcel commented

2026-03-27 17:53:32 +01:00

Problem

The document viewer restores panel state (open/closed, height, active tab) from localStorage inside an onMount-gated $effect. This runs after the first render, causing a visible flash where the panel briefly appears in its default state before snapping to the user's saved state.

Fix

Replace the localStorage restore logic with SvelteKit's native snapshot API, which restores state before the first render on browser back/forward navigation:

// routes/documents/[id]/+page.svelte

export const snapshot = {
  capture: () => ({
    panelOpen,
    activeTab,
    panelHeight
  }),
  restore: (state) => {
    panelOpen = state.panelOpen;
    activeTab = state.activeTab;
    panelHeight = state.panelHeight;
  }
};

For first-visit persistence (not just back/forward), keep a single localStorage.getItem in the initial $state declaration:

let panelOpen = $state(localStorage?.getItem(LS_KEY_OPEN) === 'true' ?? false);

This removes the localStorageRestored flag and the guarded $effect.

Acceptance Criteria

No visible flash of panel default state on page load
Panel state survives browser back/forward navigation
localStorageRestored flag and related $effect guard are removed
Behaviour is unchanged for users visiting the page for the first time

## Problem The document viewer restores panel state (open/closed, height, active tab) from `localStorage` inside an `onMount`-gated `$effect`. This runs after the first render, causing a visible flash where the panel briefly appears in its default state before snapping to the user's saved state. ## Fix Replace the `localStorage` restore logic with SvelteKit's native [snapshot API](https://kit.svelte.dev/docs/snapshots), which restores state before the first render on browser back/forward navigation: ```typescript // routes/documents/[id]/+page.svelte export const snapshot = { capture: () => ({ panelOpen, activeTab, panelHeight }), restore: (state) => { panelOpen = state.panelOpen; activeTab = state.activeTab; panelHeight = state.panelHeight; } }; ``` For first-visit persistence (not just back/forward), keep a single `localStorage.getItem` in the initial `$state` declaration: ```typescript let panelOpen = $state(localStorage?.getItem(LS_KEY_OPEN) === 'true' ?? false); ``` This removes the `localStorageRestored` flag and the guarded `$effect`. ## Acceptance Criteria - No visible flash of panel default state on page load - Panel state survives browser back/forward navigation - `localStorageRestored` flag and related `$effect` guard are removed - Behaviour is unchanged for users visiting the page for the first time

marcel added the bug ui labels 2026-03-31 20:49:45 +02:00

marcel commented

2026-03-31 20:59:16 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

The hybrid approach (snapshot for back/forward + localStorage for first-visit) is sound, but the $state(localStorage?.getItem(LS_KEY_OPEN) === 'true' ?? false) initialiser will throw on SSR because localStorage is undefined server-side. Use the browser guard from $app/environment:
```
import { browser } from '$app/environment';
let panelOpen = $state(browser ? localStorage.getItem(LS_KEY_OPEN) === 'true' : false);
```
The snapshot export must be at the top level of the <script> block in +page.svelte, not inside a function — SvelteKit requires it to be a module-level export. Double-check if the current code has it in the right place.
The localStorageRestored flag and its guarding $effect should be deleted entirely — not commented out, deleted. Per the clean code rules: no dead code.

Suggestions

The snapshot capture and restore functions should be typed:

export const snapshot: Snapshot<{ panelOpen: boolean; activeTab: string; panelHeight: number }> = { ... };

This catches shape mismatches at compile time.

Test: a Playwright test that opens the panel, navigates away, presses back, and asserts the panel is still open. Unit tests cannot cover this — it requires a real browser navigation.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - The hybrid approach (snapshot for back/forward + localStorage for first-visit) is sound, but the `$state(localStorage?.getItem(LS_KEY_OPEN) === 'true' ?? false)` initialiser will throw on SSR because `localStorage` is undefined server-side. Use the `browser` guard from `$app/environment`: ```typescript import { browser } from '$app/environment'; let panelOpen = $state(browser ? localStorage.getItem(LS_KEY_OPEN) === 'true' : false); ``` - The `snapshot` export must be at the top level of the `<script>` block in `+page.svelte`, not inside a function — SvelteKit requires it to be a module-level export. Double-check if the current code has it in the right place. - The `localStorageRestored` flag and its guarding `$effect` should be deleted entirely — not commented out, deleted. Per the clean code rules: no dead code. ### Suggestions - The snapshot `capture` and `restore` functions should be typed: ```typescript export const snapshot: Snapshot<{ panelOpen: boolean; activeTab: string; panelHeight: number }> = { ... }; ``` This catches shape mismatches at compile time. - Test: a Playwright test that opens the panel, navigates away, presses back, and asserts the panel is still open. Unit tests cannot cover this — it requires a real browser navigation.

marcel commented

2026-03-31 21:01:10 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Questions & Observations

SvelteKit snapshots use sessionStorage under the hood, not localStorage. sessionStorage is scoped to the browser tab and cleared when the tab closes — it doesn't persist across sessions, only across same-tab back/forward navigation. This is actually more private than localStorage for snapshot state, which is the right trade-off here.
localStorage remains in use for first-visit persistence. The panel open/close state is not sensitive, so storing it in localStorage is fine. But worth being explicit in the code comment: if more sensitive state (e.g. document viewing position, annotation content) is added to this persistence layer later, it should be reviewed.
No security concerns with this change. The threat model doesn't change — both localStorage and sessionStorage are readable by same-origin JS, and the state being stored (panel open, active tab, panel height) carries no sensitive data.

No security findings — clean from my perspective.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Questions & Observations - **SvelteKit snapshots use `sessionStorage` under the hood**, not `localStorage`. `sessionStorage` is scoped to the browser tab and cleared when the tab closes — it doesn't persist across sessions, only across same-tab back/forward navigation. This is actually *more* private than `localStorage` for snapshot state, which is the right trade-off here. - **`localStorage` remains in use for first-visit persistence.** The panel open/close state is not sensitive, so storing it in `localStorage` is fine. But worth being explicit in the code comment: if more sensitive state (e.g. document viewing position, annotation content) is added to this persistence layer later, it should be reviewed. - **No security concerns with this change.** The threat model doesn't change — both `localStorage` and `sessionStorage` are readable by same-origin JS, and the state being stored (panel open, active tab, panel height) carries no sensitive data. No security findings — clean from my perspective.

marcel commented

2026-03-31 21:03:01 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Test Strategy

The snapshot API requires a real browser navigation — this cannot be tested with Vitest unit tests. Playwright is the right layer.

E2E — Playwright:

test('panel state is restored after back navigation', async ({ page }) => {
  await page.goto('/documents/some-id');
  // Open the panel
  await page.getByRole('button', { name: /panel/i }).click();
  await expect(page.locator('[data-testid="side-panel"]')).toBeVisible();

  // Navigate away and back
  await page.goto('/');
  await page.goBack();

  // Panel should still be open — no flash, no reset
  await expect(page.locator('[data-testid="side-panel"]')).toBeVisible();
});

test('panel is closed by default on first visit', async ({ page }) => {
  await page.goto('/documents/some-id');
  await expect(page.locator('[data-testid="side-panel"]')).not.toBeVisible();
});

Edge Cases to Cover

Panel state after a full page reload (not back navigation) — should read from localStorage, not snapshot
Panel state after closing and reopening the browser tab — snapshot is gone, localStorage persists

Observations

Add data-testid attributes to the panel and its toggle button if they're not already there — otherwise the Playwright test has no stable selector to work with.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Test Strategy The snapshot API requires a real browser navigation — this cannot be tested with Vitest unit tests. Playwright is the right layer. **E2E — Playwright:** ```typescript test('panel state is restored after back navigation', async ({ page }) => { await page.goto('/documents/some-id'); // Open the panel await page.getByRole('button', { name: /panel/i }).click(); await expect(page.locator('[data-testid="side-panel"]')).toBeVisible(); // Navigate away and back await page.goto('/'); await page.goBack(); // Panel should still be open — no flash, no reset await expect(page.locator('[data-testid="side-panel"]')).toBeVisible(); }); test('panel is closed by default on first visit', async ({ page }) => { await page.goto('/documents/some-id'); await expect(page.locator('[data-testid="side-panel"]')).not.toBeVisible(); }); ``` ### Edge Cases to Cover - Panel state after a full page reload (not back navigation) — should read from `localStorage`, not snapshot - Panel state after closing and reopening the browser tab — snapshot is gone, `localStorage` persists ### Observations - Add `data-testid` attributes to the panel and its toggle button if they're not already there — otherwise the Playwright test has no stable selector to work with.

marcel commented

2026-03-31 21:05:01 +02:00

🏗️ Markus Keller — Application Architect

Questions & Observations

The snapshot API is exactly the right tool. It is SvelteKit's native answer to "persist state across same-tab navigation" — using onMount + localStorage for this was always a workaround for the lack of a better API. The snapshot API makes the intent explicit and moves the complexity into the framework where it belongs.
The hybrid approach is correct but warrants a comment. Two persistence mechanisms (snapshot for navigation, localStorage for first-visit) serving two different use cases in the same component can confuse future developers. A short comment in the code explaining why both are needed is justified here — this is one of the rare cases where a "why" comment adds real value.
SSR guard is required. The localStorage?.getItem() optional chain does not protect against SSR — in server-side rendering, localStorage doesn't exist at all and optional chaining on an undefined global won't help. Use import { browser } from '$app/environment' as the guard. This is not optional.

Suggestions

The Snapshot type import from @sveltejs/kit makes the export typed and checked at compile time — use it as Felix suggests.
Delete localStorageRestored and its $effect entirely. Do not leave them commented out. The version history is in git.

## 🏗️ Markus Keller — Application Architect ### Questions & Observations - **The snapshot API is exactly the right tool.** It is SvelteKit's native answer to "persist state across same-tab navigation" — using `onMount` + `localStorage` for this was always a workaround for the lack of a better API. The snapshot API makes the intent explicit and moves the complexity into the framework where it belongs. - **The hybrid approach is correct but warrants a comment.** Two persistence mechanisms (snapshot for navigation, localStorage for first-visit) serving two different use cases in the same component can confuse future developers. A short comment in the code explaining why both are needed is justified here — this is one of the rare cases where a "why" comment adds real value. - **SSR guard is required.** The `localStorage?.getItem()` optional chain does not protect against SSR — in server-side rendering, `localStorage` doesn't exist at all and optional chaining on an undefined global won't help. Use `import { browser } from '$app/environment'` as the guard. This is not optional. ### Suggestions - The `Snapshot` type import from `@sveltejs/kit` makes the export typed and checked at compile time — use it as Felix suggests. - Delete `localStorageRestored` and its `$effect` entirely. Do not leave them commented out. The version history is in git.

marcel commented

2026-03-31 21:12:26 +02:00

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

Questions & Observations

The flash is a real problem, not just a polish issue. On first load, the panel briefly appearing in default state before snapping to the saved state is disorienting — particularly for users on slower connections or older devices. For a senior user who has carefully configured their preferred panel layout, seeing it reset on every page load undermines trust in the application.
No visual design changes are needed for this fix. The snapshot API is a behaviour improvement, not a visual change — the panel looks identical before and after; it just starts in the right state.

Suggestions

After the fix, verify at mobile viewport (320px) that the panel default closed state renders correctly on first visit — no jump, no flash, and the panel opens smoothly when triggered.
If a loading skeleton or placeholder state is needed while state restores, this would be the right moment to consider one — but only if a flash is still perceptible after the snapshot fix. Don't add complexity pre-emptively.

## 🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist ### Questions & Observations - **The flash is a real problem, not just a polish issue.** On first load, the panel briefly appearing in default state before snapping to the saved state is disorienting — particularly for users on slower connections or older devices. For a senior user who has carefully configured their preferred panel layout, seeing it reset on every page load undermines trust in the application. - **No visual design changes are needed for this fix.** The snapshot API is a behaviour improvement, not a visual change — the panel looks identical before and after; it just starts in the right state. ### Suggestions - After the fix, verify at mobile viewport (320px) that the panel default closed state renders correctly on first visit — no jump, no flash, and the panel opens smoothly when triggered. - If a loading skeleton or placeholder state is needed while state restores, this would be the right moment to consider one — but only if a flash is still perceptible after the snapshot fix. Don't add complexity pre-emptively.

marcel commented

2026-03-31 21:13:50 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

No infrastructure concerns — pure frontend change, no deployment or config impact.

One build note: the browser guard from $app/environment is a SvelteKit build-time constant — it tree-shakes cleanly in production builds, so the SSR path has zero overhead from the guard. No performance concern there.

The Playwright test for snapshot restore (back-navigation) will add a small amount of time to the E2E job. It's worth it — this is the only layer that can catch the flash regression.

## 🚀 Tobias Wendt — DevOps & Platform Engineer No infrastructure concerns — pure frontend change, no deployment or config impact. One build note: the `browser` guard from `$app/environment` is a SvelteKit build-time constant — it tree-shakes cleanly in production builds, so the SSR path has zero overhead from the guard. No performance concern there. The Playwright test for snapshot restore (back-navigation) will add a small amount of time to the E2E job. It's worth it — this is the only layer that can catch the flash regression.

marcel closed this issue

2026-05-09 16:11:20 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#115