From 4f48ecd4bc796a43fc16c34f0f7acad400345ac2 Mon Sep 17 00:00:00 2001
From: JasonOA888 <JasonOA888@users.noreply.github.com>
Date: Sat, 4 Apr 2026 23:58:25 +0800
Subject: [PATCH] fix: address code review feedback for settings persistence

- Replace useRef with useState for prefsHydrated to prevent race condition
- Wrap localStorage.getItem in try/catch in loadUserPreferences
- Validate aspectRatio against known valid values
- Include 'good' in exportQuality validation, 'mp4' in exportFormat validation
---
 package-lock.json                           |  4 +--
 src/components/video-editor/VideoEditor.tsx |  8 +++---
 src/lib/userPreferences.ts                  | 28 ++++++++++++++++++---
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/package-lock.json b/package-lock.json
index 70e3395..fdbd6b9 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,12 +1,12 @@
 {
 	"name": "openscreen",
-	"version": "1.2.0",
+	"version": "1.3.0",
 	"lockfileVersion": 3,
 	"requires": true,
 	"packages": {
 		"": {
 			"name": "openscreen",
-			"version": "1.2.0",
+			"version": "1.3.0",
 			"dependencies": {
 				"@fix-webm-duration/fix": "^1.0.1",
 				"@pixi/filter-drop-shadow": "^5.2.0",
diff --git a/src/components/video-editor/VideoEditor.tsx b/src/components/video-editor/VideoEditor.tsx
index 6d7c5c5..4168ef8 100644
--- a/src/components/video-editor/VideoEditor.tsx
+++ b/src/components/video-editor/VideoEditor.tsx
@@ -362,7 +362,7 @@ export default function VideoEditor() {
 
 	// Track whether user preferences have been loaded to avoid
 	// overwriting saved prefs with defaults on the first render
-	const prefsLoadedRef = useRef(false);
+	const [prefsHydrated, setPrefsHydrated] = useState(false);
 
 	// Load persisted user preferences on mount
 	useEffect(() => {
@@ -373,16 +373,16 @@ export default function VideoEditor() {
 		});
 		setExportQuality(prefs.exportQuality);
 		setExportFormat(prefs.exportFormat);
-		prefsLoadedRef.current = true;
+		setPrefsHydrated(true);
 		// We intentionally only want this to run once on mount
 		// biome-ignore lint/correctness/useExhaustiveDependencies: mount-only effect
 	}, []);
 
 	// Auto-save user preferences when settings change
 	useEffect(() => {
-		if (!prefsLoadedRef.current) return;
+		if (!prefsHydrated) return;
 		saveUserPreferences({ padding, aspectRatio, exportQuality, exportFormat });
-	}, [padding, aspectRatio, exportQuality, exportFormat]);
+	}, [prefsHydrated, padding, aspectRatio, exportQuality, exportFormat]);
 
 	const saveProject = useCallback(
 		async (forceSaveAs: boolean) => {
diff --git a/src/lib/userPreferences.ts b/src/lib/userPreferences.ts
index ae9d14f..5839799 100644
--- a/src/lib/userPreferences.ts
+++ b/src/lib/userPreferences.ts
@@ -3,6 +3,17 @@ import type { AspectRatio } from "@/utils/aspectRatioUtils";
 
 const PREFS_KEY = "openscreen_user_preferences";
 
+const VALID_ASPECT_RATIOS: readonly string[] = [
+	"16:9",
+	"9:16",
+	"1:1",
+	"4:3",
+	"4:5",
+	"16:10",
+	"10:16",
+	"native",
+];
+
 export interface UserPreferences {
 	/** Default padding % */
 	padding: number;
@@ -35,7 +46,12 @@ function safeJsonParse(text: string | null): Record<string, unknown> | null {
  * Returns defaults for any missing or invalid fields.
  */
 export function loadUserPreferences(): UserPreferences {
-	const raw = safeJsonParse(localStorage.getItem(PREFS_KEY));
+	let raw: Record<string, unknown> | null = null;
+	try {
+		raw = safeJsonParse(localStorage.getItem(PREFS_KEY));
+	} catch {
+		return { ...DEFAULT_PREFS };
+	}
 	if (!raw || typeof raw !== "object") return { ...DEFAULT_PREFS };
 
 	return {
@@ -44,13 +60,17 @@ export function loadUserPreferences(): UserPreferences {
 				? raw.padding
 				: DEFAULT_PREFS.padding,
 		aspectRatio:
-			typeof raw.aspectRatio === "string" ? (raw.aspectRatio as AspectRatio) : DEFAULT_PREFS.aspectRatio,
+			typeof raw.aspectRatio === "string" && VALID_ASPECT_RATIOS.includes(raw.aspectRatio)
+				? (raw.aspectRatio as AspectRatio)
+				: DEFAULT_PREFS.aspectRatio,
 		exportQuality:
-			raw.exportQuality === "medium" || raw.exportQuality === "source"
+			raw.exportQuality === "medium" || raw.exportQuality === "good" || raw.exportQuality === "source"
 				? (raw.exportQuality as ExportQuality)
 				: DEFAULT_PREFS.exportQuality,
 		exportFormat:
-			raw.exportFormat === "gif" ? (raw.exportFormat as ExportFormat) : DEFAULT_PREFS.exportFormat,
+			raw.exportFormat === "gif" || raw.exportFormat === "mp4"
+				? (raw.exportFormat as ExportFormat)
+				: DEFAULT_PREFS.exportFormat,
 	};
 }