From d72d32001e922299bdda4ab50436133da1b01d73 Mon Sep 17 00:00:00 2001 From: Justin Edmund Date: Mon, 24 Nov 2025 08:03:43 -0800 Subject: [PATCH] fix: Address critical issues in admin form refactor Phase 1: Critical Type Fixes - Fix useDraftRecovery type signature to accept () => string | null (was incorrectly typed as string | null) - Add TResponse generic parameter to AutoSaveStore interface - Update all call sites to use function for draftKey reactivity Phase 2: Robustness Improvements - Remove non-null assertions in useFormGuards - Capture autoSave in closures to prevent potential null access - Make useFormGuards generic to accept any AutoSaveStore types Documentation & Code Quality - Document AlbumForm autosave initialization order - Add comments explaining void operator usage for reactivity - Fix eslint error: prefix unused _TResponse with underscore These fixes address the critical issues found in PR review: 1. Type mismatch causing TypeScript errors 2. Non-null assertions that could cause crashes 3. Missing documentation for complex initialization patterns --- src/lib/admin/autoSave.svelte.ts | 4 +-- src/lib/admin/useDraftRecovery.svelte.ts | 19 ++++++----- src/lib/admin/useFormGuards.svelte.ts | 16 +++++++-- src/lib/components/admin/AlbumForm.svelte | 41 +++++++++++++++++------ 4 files changed, 57 insertions(+), 23 deletions(-) diff --git a/src/lib/admin/autoSave.svelte.ts b/src/lib/admin/autoSave.svelte.ts index 6e5bf3c..d4b50e6 100644 --- a/src/lib/admin/autoSave.svelte.ts +++ b/src/lib/admin/autoSave.svelte.ts @@ -8,7 +8,7 @@ export interface AutoSaveStoreOptions { onSaved?: (res: TResponse, ctx: { prime: (payload: TPayload) => void }) => void } -export interface AutoSaveStore { +export interface AutoSaveStore { readonly status: AutoSaveStatus readonly lastError: string | null schedule: () => void @@ -36,7 +36,7 @@ export interface AutoSaveStore { */ export function createAutoSaveStore( opts: AutoSaveStoreOptions -): AutoSaveStore { +): AutoSaveStore { const debounceMs = opts.debounceMs ?? 2000 const idleResetMs = opts.idleResetMs ?? 2000 let timer: ReturnType | null = null diff --git a/src/lib/admin/useDraftRecovery.svelte.ts b/src/lib/admin/useDraftRecovery.svelte.ts index 77d6f75..c8f84df 100644 --- a/src/lib/admin/useDraftRecovery.svelte.ts +++ b/src/lib/admin/useDraftRecovery.svelte.ts @@ -1,7 +1,7 @@ import { loadDraft, clearDraft, timeAgo } from '$lib/admin/draftStore' export function useDraftRecovery(options: { - draftKey: string | null + draftKey: () => string | null onRestore: (payload: TPayload) => void enabled?: boolean }) { @@ -17,9 +17,10 @@ export function useDraftRecovery(options: { // Auto-detect draft on mount using $effect $effect(() => { - if (!options.draftKey || options.enabled === false) return + const key = options.draftKey() + if (!key || options.enabled === false) return - const draft = loadDraft(options.draftKey) + const draft = loadDraft(key) if (draft) { showPrompt = true draftTimestamp = draft.ts @@ -43,19 +44,21 @@ export function useDraftRecovery(options: { draftTimeText, restore() { - if (!options.draftKey) return - const draft = loadDraft(options.draftKey) + const key = options.draftKey() + if (!key) return + const draft = loadDraft(key) if (!draft) return options.onRestore(draft.payload) showPrompt = false - clearDraft(options.draftKey) + clearDraft(key) }, dismiss() { - if (!options.draftKey) return + const key = options.draftKey() + if (!key) return showPrompt = false - clearDraft(options.draftKey) + clearDraft(key) } } } diff --git a/src/lib/admin/useFormGuards.svelte.ts b/src/lib/admin/useFormGuards.svelte.ts index 0d84759..3b3ac9f 100644 --- a/src/lib/admin/useFormGuards.svelte.ts +++ b/src/lib/admin/useFormGuards.svelte.ts @@ -2,7 +2,9 @@ import { beforeNavigate } from '$app/navigation' import { toast } from '$lib/stores/toast' import type { AutoSaveStore } from '$lib/admin/autoSave.svelte' -export function useFormGuards(autoSave: AutoSaveStore | null) { +export function useFormGuards( + autoSave: AutoSaveStore | null +) { if (!autoSave) return // No guards needed for create mode // Navigation guard: flush autosave before route change @@ -21,8 +23,12 @@ export function useFormGuards(autoSave: AutoSaveStore | null) // Warn before closing browser tab/window if unsaved changes $effect(() => { + // Capture autoSave in closure to avoid non-null assertions + const store = autoSave + if (!store) return + function handleBeforeUnload(event: BeforeUnloadEvent) { - if (autoSave!.status !== 'saved') { + if (store.status !== 'saved') { event.preventDefault() event.returnValue = '' } @@ -34,13 +40,17 @@ export function useFormGuards(autoSave: AutoSaveStore | null) // Cmd/Ctrl+S keyboard shortcut for immediate save $effect(() => { + // Capture autoSave in closure to avoid non-null assertions + const store = autoSave + if (!store) return + function handleKeydown(event: KeyboardEvent) { const key = event.key.toLowerCase() const isModifier = event.metaKey || event.ctrlKey if (isModifier && key === 's') { event.preventDefault() - autoSave!.flush().catch((error) => { + store.flush().catch((error) => { console.error('Autosave flush failed:', error) toast.error('Failed to save changes') }) diff --git a/src/lib/components/admin/AlbumForm.svelte b/src/lib/components/admin/AlbumForm.svelte index 07c449b..20218ca 100644 --- a/src/lib/components/admin/AlbumForm.svelte +++ b/src/lib/components/admin/AlbumForm.svelte @@ -46,7 +46,13 @@ let editorInstance = $state<{ save: () => Promise; clear: () => void } | undefined>() let activeTab = $state('metadata') let pendingMediaIds = $state([]) // Photos to add after album creation - let updatedAt = $state(album?.updatedAt?.toISOString()) + let updatedAt = $state( + album?.updatedAt + ? typeof album.updatedAt === 'string' + ? album.updatedAt + : album.updatedAt.toISOString() + : undefined + ) const tabOptions = [ { value: 'metadata', label: 'Metadata' }, @@ -98,12 +104,22 @@ } // Autosave store (edit mode only) - const autoSave = mode === 'edit' && album - ? createAutoSaveStore({ + // Initialized as null and created reactively when album data becomes available + let autoSave = $state, Album>> | null>(null) + + // INITIALIZATION ORDER: + // 1. This effect creates autoSave when album prop becomes available + // 2. useFormGuards is called immediately after creation (same effect) + // 3. Other effects check for autoSave existence before using it + $effect(() => { + // Create autoSave when album becomes available (only once) + if (mode === 'edit' && album && !autoSave) { + const albumId = album.id // Capture album ID to avoid null reference + autoSave = createAutoSaveStore({ debounceMs: 2000, getPayload: () => (hasLoaded ? buildPayload() : null), save: async (payload, { signal }) => { - const response = await fetch(`/api/albums/${album.id}`, { + const response = await fetch(`/api/albums/${albumId}`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(payload), @@ -114,12 +130,17 @@ return await response.json() }, onSaved: (saved: Album, { prime }) => { - updatedAt = saved.updatedAt.toISOString() + updatedAt = + typeof saved.updatedAt === 'string' ? saved.updatedAt : saved.updatedAt.toISOString() prime(buildPayload()) if (draftKey) clearDraft(draftKey) } }) - : null + + // Form guards (navigation protection, Cmd+S, beforeunload) + useFormGuards(autoSave) + } + }) // Draft recovery helper const draftRecovery = useDraftRecovery>({ @@ -135,9 +156,6 @@ } }) - // Form guards (navigation protection, Cmd+S, beforeunload) - useFormGuards(autoSave) - // Watch for album changes and populate form data $effect(() => { if (album && mode === 'edit') { @@ -167,6 +185,8 @@ }) // Trigger autosave when form data changes + // Using `void` operator to explicitly track dependencies without using their values + // This effect re-runs whenever any of these form fields change $effect(() => { void formData.title void formData.slug @@ -194,7 +214,8 @@ // Cleanup autosave on unmount $effect(() => { if (autoSave) { - return () => autoSave.destroy() + const instance = autoSave + return () => instance.destroy() } })