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
This commit is contained in:
parent
5e58d31f7e
commit
d72d32001e
4 changed files with 57 additions and 23 deletions
|
|
@ -8,7 +8,7 @@ export interface AutoSaveStoreOptions<TPayload, TResponse = unknown> {
|
||||||
onSaved?: (res: TResponse, ctx: { prime: (payload: TPayload) => void }) => void
|
onSaved?: (res: TResponse, ctx: { prime: (payload: TPayload) => void }) => void
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface AutoSaveStore<TPayload> {
|
export interface AutoSaveStore<TPayload, _TResponse = unknown> {
|
||||||
readonly status: AutoSaveStatus
|
readonly status: AutoSaveStatus
|
||||||
readonly lastError: string | null
|
readonly lastError: string | null
|
||||||
schedule: () => void
|
schedule: () => void
|
||||||
|
|
@ -36,7 +36,7 @@ export interface AutoSaveStore<TPayload> {
|
||||||
*/
|
*/
|
||||||
export function createAutoSaveStore<TPayload, TResponse = unknown>(
|
export function createAutoSaveStore<TPayload, TResponse = unknown>(
|
||||||
opts: AutoSaveStoreOptions<TPayload, TResponse>
|
opts: AutoSaveStoreOptions<TPayload, TResponse>
|
||||||
): AutoSaveStore<TPayload> {
|
): AutoSaveStore<TPayload, unknown> {
|
||||||
const debounceMs = opts.debounceMs ?? 2000
|
const debounceMs = opts.debounceMs ?? 2000
|
||||||
const idleResetMs = opts.idleResetMs ?? 2000
|
const idleResetMs = opts.idleResetMs ?? 2000
|
||||||
let timer: ReturnType<typeof setTimeout> | null = null
|
let timer: ReturnType<typeof setTimeout> | null = null
|
||||||
|
|
|
||||||
|
|
@ -1,7 +1,7 @@
|
||||||
import { loadDraft, clearDraft, timeAgo } from '$lib/admin/draftStore'
|
import { loadDraft, clearDraft, timeAgo } from '$lib/admin/draftStore'
|
||||||
|
|
||||||
export function useDraftRecovery<TPayload>(options: {
|
export function useDraftRecovery<TPayload>(options: {
|
||||||
draftKey: string | null
|
draftKey: () => string | null
|
||||||
onRestore: (payload: TPayload) => void
|
onRestore: (payload: TPayload) => void
|
||||||
enabled?: boolean
|
enabled?: boolean
|
||||||
}) {
|
}) {
|
||||||
|
|
@ -17,9 +17,10 @@ export function useDraftRecovery<TPayload>(options: {
|
||||||
|
|
||||||
// Auto-detect draft on mount using $effect
|
// Auto-detect draft on mount using $effect
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
if (!options.draftKey || options.enabled === false) return
|
const key = options.draftKey()
|
||||||
|
if (!key || options.enabled === false) return
|
||||||
|
|
||||||
const draft = loadDraft<TPayload>(options.draftKey)
|
const draft = loadDraft<TPayload>(key)
|
||||||
if (draft) {
|
if (draft) {
|
||||||
showPrompt = true
|
showPrompt = true
|
||||||
draftTimestamp = draft.ts
|
draftTimestamp = draft.ts
|
||||||
|
|
@ -43,19 +44,21 @@ export function useDraftRecovery<TPayload>(options: {
|
||||||
draftTimeText,
|
draftTimeText,
|
||||||
|
|
||||||
restore() {
|
restore() {
|
||||||
if (!options.draftKey) return
|
const key = options.draftKey()
|
||||||
const draft = loadDraft<TPayload>(options.draftKey)
|
if (!key) return
|
||||||
|
const draft = loadDraft<TPayload>(key)
|
||||||
if (!draft) return
|
if (!draft) return
|
||||||
|
|
||||||
options.onRestore(draft.payload)
|
options.onRestore(draft.payload)
|
||||||
showPrompt = false
|
showPrompt = false
|
||||||
clearDraft(options.draftKey)
|
clearDraft(key)
|
||||||
},
|
},
|
||||||
|
|
||||||
dismiss() {
|
dismiss() {
|
||||||
if (!options.draftKey) return
|
const key = options.draftKey()
|
||||||
|
if (!key) return
|
||||||
showPrompt = false
|
showPrompt = false
|
||||||
clearDraft(options.draftKey)
|
clearDraft(key)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -2,7 +2,9 @@ import { beforeNavigate } from '$app/navigation'
|
||||||
import { toast } from '$lib/stores/toast'
|
import { toast } from '$lib/stores/toast'
|
||||||
import type { AutoSaveStore } from '$lib/admin/autoSave.svelte'
|
import type { AutoSaveStore } from '$lib/admin/autoSave.svelte'
|
||||||
|
|
||||||
export function useFormGuards(autoSave: AutoSaveStore<unknown, unknown> | null) {
|
export function useFormGuards<TPayload = unknown, _TResponse = unknown>(
|
||||||
|
autoSave: AutoSaveStore<TPayload, unknown> | null
|
||||||
|
) {
|
||||||
if (!autoSave) return // No guards needed for create mode
|
if (!autoSave) return // No guards needed for create mode
|
||||||
|
|
||||||
// Navigation guard: flush autosave before route change
|
// Navigation guard: flush autosave before route change
|
||||||
|
|
@ -21,8 +23,12 @@ export function useFormGuards(autoSave: AutoSaveStore<unknown, unknown> | null)
|
||||||
|
|
||||||
// Warn before closing browser tab/window if unsaved changes
|
// Warn before closing browser tab/window if unsaved changes
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
|
// Capture autoSave in closure to avoid non-null assertions
|
||||||
|
const store = autoSave
|
||||||
|
if (!store) return
|
||||||
|
|
||||||
function handleBeforeUnload(event: BeforeUnloadEvent) {
|
function handleBeforeUnload(event: BeforeUnloadEvent) {
|
||||||
if (autoSave!.status !== 'saved') {
|
if (store.status !== 'saved') {
|
||||||
event.preventDefault()
|
event.preventDefault()
|
||||||
event.returnValue = ''
|
event.returnValue = ''
|
||||||
}
|
}
|
||||||
|
|
@ -34,13 +40,17 @@ export function useFormGuards(autoSave: AutoSaveStore<unknown, unknown> | null)
|
||||||
|
|
||||||
// Cmd/Ctrl+S keyboard shortcut for immediate save
|
// Cmd/Ctrl+S keyboard shortcut for immediate save
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
|
// Capture autoSave in closure to avoid non-null assertions
|
||||||
|
const store = autoSave
|
||||||
|
if (!store) return
|
||||||
|
|
||||||
function handleKeydown(event: KeyboardEvent) {
|
function handleKeydown(event: KeyboardEvent) {
|
||||||
const key = event.key.toLowerCase()
|
const key = event.key.toLowerCase()
|
||||||
const isModifier = event.metaKey || event.ctrlKey
|
const isModifier = event.metaKey || event.ctrlKey
|
||||||
|
|
||||||
if (isModifier && key === 's') {
|
if (isModifier && key === 's') {
|
||||||
event.preventDefault()
|
event.preventDefault()
|
||||||
autoSave!.flush().catch((error) => {
|
store.flush().catch((error) => {
|
||||||
console.error('Autosave flush failed:', error)
|
console.error('Autosave flush failed:', error)
|
||||||
toast.error('Failed to save changes')
|
toast.error('Failed to save changes')
|
||||||
})
|
})
|
||||||
|
|
|
||||||
|
|
@ -46,7 +46,13 @@
|
||||||
let editorInstance = $state<{ save: () => Promise<JSONContent>; clear: () => void } | undefined>()
|
let editorInstance = $state<{ save: () => Promise<JSONContent>; clear: () => void } | undefined>()
|
||||||
let activeTab = $state('metadata')
|
let activeTab = $state('metadata')
|
||||||
let pendingMediaIds = $state<number[]>([]) // Photos to add after album creation
|
let pendingMediaIds = $state<number[]>([]) // Photos to add after album creation
|
||||||
let updatedAt = $state<string | undefined>(album?.updatedAt?.toISOString())
|
let updatedAt = $state<string | undefined>(
|
||||||
|
album?.updatedAt
|
||||||
|
? typeof album.updatedAt === 'string'
|
||||||
|
? album.updatedAt
|
||||||
|
: album.updatedAt.toISOString()
|
||||||
|
: undefined
|
||||||
|
)
|
||||||
|
|
||||||
const tabOptions = [
|
const tabOptions = [
|
||||||
{ value: 'metadata', label: 'Metadata' },
|
{ value: 'metadata', label: 'Metadata' },
|
||||||
|
|
@ -98,12 +104,22 @@
|
||||||
}
|
}
|
||||||
|
|
||||||
// Autosave store (edit mode only)
|
// Autosave store (edit mode only)
|
||||||
const autoSave = mode === 'edit' && album
|
// Initialized as null and created reactively when album data becomes available
|
||||||
? createAutoSaveStore({
|
let autoSave = $state<ReturnType<typeof createAutoSaveStore<ReturnType<typeof buildPayload>, 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,
|
debounceMs: 2000,
|
||||||
getPayload: () => (hasLoaded ? buildPayload() : null),
|
getPayload: () => (hasLoaded ? buildPayload() : null),
|
||||||
save: async (payload, { signal }) => {
|
save: async (payload, { signal }) => {
|
||||||
const response = await fetch(`/api/albums/${album.id}`, {
|
const response = await fetch(`/api/albums/${albumId}`, {
|
||||||
method: 'PUT',
|
method: 'PUT',
|
||||||
headers: { 'Content-Type': 'application/json' },
|
headers: { 'Content-Type': 'application/json' },
|
||||||
body: JSON.stringify(payload),
|
body: JSON.stringify(payload),
|
||||||
|
|
@ -114,12 +130,17 @@
|
||||||
return await response.json()
|
return await response.json()
|
||||||
},
|
},
|
||||||
onSaved: (saved: Album, { prime }) => {
|
onSaved: (saved: Album, { prime }) => {
|
||||||
updatedAt = saved.updatedAt.toISOString()
|
updatedAt =
|
||||||
|
typeof saved.updatedAt === 'string' ? saved.updatedAt : saved.updatedAt.toISOString()
|
||||||
prime(buildPayload())
|
prime(buildPayload())
|
||||||
if (draftKey) clearDraft(draftKey)
|
if (draftKey) clearDraft(draftKey)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
: null
|
|
||||||
|
// Form guards (navigation protection, Cmd+S, beforeunload)
|
||||||
|
useFormGuards(autoSave)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
// Draft recovery helper
|
// Draft recovery helper
|
||||||
const draftRecovery = useDraftRecovery<ReturnType<typeof buildPayload>>({
|
const draftRecovery = useDraftRecovery<ReturnType<typeof buildPayload>>({
|
||||||
|
|
@ -135,9 +156,6 @@
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
// Form guards (navigation protection, Cmd+S, beforeunload)
|
|
||||||
useFormGuards(autoSave)
|
|
||||||
|
|
||||||
// Watch for album changes and populate form data
|
// Watch for album changes and populate form data
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
if (album && mode === 'edit') {
|
if (album && mode === 'edit') {
|
||||||
|
|
@ -167,6 +185,8 @@
|
||||||
})
|
})
|
||||||
|
|
||||||
// Trigger autosave when form data changes
|
// 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(() => {
|
$effect(() => {
|
||||||
void formData.title
|
void formData.title
|
||||||
void formData.slug
|
void formData.slug
|
||||||
|
|
@ -194,7 +214,8 @@
|
||||||
// Cleanup autosave on unmount
|
// Cleanup autosave on unmount
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
if (autoSave) {
|
if (autoSave) {
|
||||||
return () => autoSave.destroy()
|
const instance = autoSave
|
||||||
|
return () => instance.destroy()
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue