From f2ff7fb21cf38b018135f82da9f96692d645ae5c Mon Sep 17 00:00:00 2001 From: Enriquefft Date: Fri, 24 Apr 2026 18:55:04 -0500 Subject: [PATCH] address review audit: persist canonical wallpaper, dedupe types, tighten edge cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R1 — Persisted wallpaper is now always the canonical /wallpapers/wallpaperN.jpg form, never the resolved file:// URL. Swatch clicks pass WALLPAPER_PATHS[i] (the relative path) to onWallpaperChange; the resolved URL stays in wallpaperPreviewUrls for rendering only. This prevents machine-specific paths from being written into project JSON and avoids break-on-upgrade / break-on-share regressions. Legacy projects carrying resolved file:// URLs are rewritten by a new normalizer in normalizeProjectEditor: file://…(/assets)?/wallpapers/wallpaperN.jpg → /wallpapers/wallpaperN.jpg. R2 — resolveImageWallpaperUrl now catches anything getAssetPath throws (UnsafeAssetPathError, AssetBaseUnavailableError) and rewraps as BackgroundLoadError with the original as cause. Callers (videoExporter retry loop, gifExporter catch, VideoEditor toast) only need one instanceof check and users always see the translated errors.exportBackgroundLoadFailed toast. R3 — src/vite-env.d.ts no longer duplicates Window.electronAPI. The interface had drifted — renderer declaration was missing readBinaryFile, getPlatform, revealInFolder, getShortcuts, saveShortcuts, hudOverlay*, countdown overlay methods that electron-env.d.ts already declares. Removed the duplicate and kept the triple-slash reference so the authoritative declaration is the one in electron/electron-env.d.ts. N1 — GRADIENT_RE accepts optional "repeating-" prefix so repeating-linear/radial/conic-gradient values classify as gradients instead of falling through to color. N2 — displayBasename returns "(unknown)" sentinel for URLs without a meaningful basename (file:///, bare /) instead of leaking the original string. N3 — electron-builder.json5 extraResources block gets an inline comment pointing at preload.ts:assetBaseDir so the bidirectional coupling is discoverable from either file. Tests: 54 unit tests pass (up from 35). New coverage for repeating gradients, displayBasename sentinels, BackgroundLoadError cause wrapping, legacy file:// wallpaper normalization (5 cases). --- electron-builder.json5 | 2 + src/components/video-editor/SettingsPanel.tsx | 30 ++--- .../video-editor/projectPersistence.test.ts | 40 ++++++ .../video-editor/projectPersistence.ts | 16 ++- src/lib/wallpaper.test.ts | 56 ++++++-- src/lib/wallpaper.ts | 12 +- src/vite-env.d.ts | 124 ------------------ 7 files changed, 120 insertions(+), 160 deletions(-) diff --git a/electron-builder.json5 b/electron-builder.json5 index 441eda4..ca053ef 100644 --- a/electron-builder.json5 +++ b/electron-builder.json5 @@ -20,6 +20,8 @@ "!CONTRIBUTING.md", "!LICENSE" ], + // Asset layout contract: "wallpapers/" under resourcesPath must align with + // assetBaseDir in electron/preload.ts (packaged branch). "extraResources": [ { "from": "public/wallpapers", diff --git a/src/components/video-editor/SettingsPanel.tsx b/src/components/video-editor/SettingsPanel.tsx index 91840bc..78d6bb4 100644 --- a/src/components/video-editor/SettingsPanel.tsx +++ b/src/components/video-editor/SettingsPanel.tsx @@ -321,7 +321,10 @@ export function SettingsPanel({ onWebcamSizePresetCommit, }: SettingsPanelProps) { const t = useScopedT("settings"); - const wallpaperPaths = useMemo(() => WALLPAPER_PATHS.map(resolveImageWallpaperUrl), []); + // Resolved URLs are for DOM rendering only (backgroundImage). The canonical + // `/wallpapers/wallpaperN.jpg` form in WALLPAPER_PATHS is what gets persisted + // on click — never the machine-specific file:// URL. + const wallpaperPreviewUrls = useMemo(() => WALLPAPER_PATHS.map(resolveImageWallpaperUrl), []); const [customImages, setCustomImages] = useState([]); const fileInputRef = useRef(null); const colorPalette = [ @@ -506,7 +509,7 @@ export function SettingsPanel({ setCustomImages((prev) => prev.filter((img) => img !== imageUrl)); // If the removed image was selected, clear selection if (selected === imageUrl) { - onWallpaperChange(wallpaperPaths[0] || WALLPAPER_PATHS[0]); + onWallpaperChange(WALLPAPER_PATHS[0]); } }; @@ -1126,23 +1129,12 @@ export function SettingsPanel({ ); })} - {wallpaperPaths.map((path) => { - const isSelected = (() => { - if (!selected) return false; - if (selected === path) return true; - try { - const clean = (s: string) => - s.replace(/^file:\/\//, "").replace(/^\//, ""); - if (clean(selected).endsWith(clean(path))) return true; - if (clean(path).endsWith(clean(selected))) return true; - } catch { - // Best-effort comparison; fallback to strict match. - } - return false; - })(); + {WALLPAPER_PATHS.map((canonicalPath, i) => { + const previewUrl = wallpaperPreviewUrls[i] ?? canonicalPath; + const isSelected = selected === canonicalPath; return (
onWallpaperChange(path)} + onClick={() => onWallpaperChange(canonicalPath)} role="button" /> ); diff --git a/src/components/video-editor/projectPersistence.test.ts b/src/components/video-editor/projectPersistence.test.ts index 14dc240..d816f48 100644 --- a/src/components/video-editor/projectPersistence.test.ts +++ b/src/components/video-editor/projectPersistence.test.ts @@ -197,3 +197,43 @@ it("detects unsaved changes from differing snapshots", () => { expect(hasProjectUnsavedChanges("same", "same")).toBe(false); expect(hasProjectUnsavedChanges("current", "baseline")).toBe(true); }); + +describe("wallpaper legacy normalization", () => { + it("rewrites resolved file:// resources paths from pre-fix projects", () => { + const normalized = normalizeProjectEditor({ + wallpaper: "file:///opt/Openscreen/resources/assets/wallpapers/wallpaper5.jpg", + }); + expect(normalized.wallpaper).toBe("/wallpapers/wallpaper5.jpg"); + }); + + it("rewrites resolved file:// paths under the new resources/wallpapers layout", () => { + const normalized = normalizeProjectEditor({ + wallpaper: "file:///opt/Openscreen/resources/wallpapers/wallpaper3.jpg", + }); + expect(normalized.wallpaper).toBe("/wallpapers/wallpaper3.jpg"); + }); + + it("rewrites unpackaged dev paths (public/wallpapers/…)", () => { + const normalized = normalizeProjectEditor({ + wallpaper: "file:///home/user/project/public/wallpapers/wallpaper1.jpg", + }); + expect(normalized.wallpaper).toBe("/wallpapers/wallpaper1.jpg"); + }); + + it("leaves canonical relative paths untouched", () => { + const normalized = normalizeProjectEditor({ wallpaper: "/wallpapers/wallpaper2.jpg" }); + expect(normalized.wallpaper).toBe("/wallpapers/wallpaper2.jpg"); + }); + + it("leaves data URIs untouched", () => { + const dataUri = "data:image/png;base64,AAA"; + expect(normalizeProjectEditor({ wallpaper: dataUri }).wallpaper).toBe(dataUri); + }); + + it("leaves colors and gradients untouched", () => { + expect(normalizeProjectEditor({ wallpaper: "#1a1a2e" }).wallpaper).toBe("#1a1a2e"); + expect( + normalizeProjectEditor({ wallpaper: "linear-gradient(90deg, red, blue)" }).wallpaper, + ).toBe("linear-gradient(90deg, red, blue)"); + }); +}); diff --git a/src/components/video-editor/projectPersistence.ts b/src/components/video-editor/projectPersistence.ts index 6d8f689..c0def97 100644 --- a/src/components/video-editor/projectPersistence.ts +++ b/src/components/video-editor/projectPersistence.ts @@ -40,6 +40,17 @@ import { const VALID_BLUR_SHAPES = new Set(["rectangle", "oval", "freehand"] as const); +// Pre-fix projects could persist resolved file:// URLs (machine-specific) instead +// of the canonical `/wallpapers/wallpaperN.jpg` form. Rewrite those on load so +// they resolve against the current install's resources directory. +const LEGACY_FILE_WALLPAPER_RE = /^file:\/\/.*?\/(?:assets\/)?wallpapers\/(wallpaper\d+\.jpg)$/i; + +function normalizeWallpaperValue(value: string): string { + const match = LEGACY_FILE_WALLPAPER_RE.exec(value); + if (!match) return value; + return `/wallpapers/${match[1]}`; +} + export { WALLPAPER_PATHS }; export const PROJECT_VERSION = 2; @@ -422,7 +433,10 @@ export function normalizeProjectEditor(editor: Partial): Pro const cropHeight = clamp(rawCropHeight, 0.01, 1 - cropY); return { - wallpaper: typeof editor.wallpaper === "string" ? editor.wallpaper : DEFAULT_WALLPAPER, + wallpaper: + typeof editor.wallpaper === "string" + ? normalizeWallpaperValue(editor.wallpaper) + : DEFAULT_WALLPAPER, shadowIntensity: typeof editor.shadowIntensity === "number" ? editor.shadowIntensity : 0, showBlur: typeof editor.showBlur === "boolean" ? editor.showBlur : false, motionBlurAmount: isFiniteNumber(editor.motionBlurAmount) diff --git a/src/lib/wallpaper.test.ts b/src/lib/wallpaper.test.ts index f1cb80b..6e1b74a 100644 --- a/src/lib/wallpaper.test.ts +++ b/src/lib/wallpaper.test.ts @@ -64,6 +64,16 @@ describe("classifyWallpaper", () => { expect(classifyWallpaper(v)).toEqual({ kind: "gradient", value: v }); }); + it("repeating-linear gradient", () => { + const v = "repeating-linear-gradient(45deg, red 0 10px, blue 10px 20px)"; + expect(classifyWallpaper(v)).toEqual({ kind: "gradient", value: v }); + }); + + it("repeating-radial gradient", () => { + const v = "repeating-radial-gradient(circle, red, blue 20px)"; + expect(classifyWallpaper(v)).toEqual({ kind: "gradient", value: v }); + }); + it("leading-slash image path", () => { expect(classifyWallpaper("/wallpapers/wallpaper1.jpg")).toEqual({ kind: "image", @@ -164,16 +174,24 @@ describe("resolveImageWallpaperUrl", () => { expect(() => resolveImageWallpaperUrl("/etc/passwd")).toThrow(BackgroundLoadError); }); - it("rejects traversal attempts", () => { - expect(() => resolveImageWallpaperUrl("/wallpapers/../etc/passwd")).toThrow( - UnsafeAssetPathError, - ); + it("wraps traversal attempts in BackgroundLoadError (preserves UnsafeAssetPathError as cause)", () => { + try { + resolveImageWallpaperUrl("/wallpapers/../etc/passwd"); + expect.fail("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(BackgroundLoadError); + expect((err as BackgroundLoadError).cause).toBeInstanceOf(UnsafeAssetPathError); + } }); - it("rejects percent-encoded traversal", () => { - expect(() => resolveImageWallpaperUrl("/wallpapers/%2e%2e/app.asar")).toThrow( - UnsafeAssetPathError, - ); + it("wraps percent-encoded traversal in BackgroundLoadError", () => { + try { + resolveImageWallpaperUrl("/wallpapers/%2e%2e/app.asar"); + expect.fail("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(BackgroundLoadError); + expect((err as BackgroundLoadError).cause).toBeInstanceOf(UnsafeAssetPathError); + } }); it("resolves via electronAPI.assetBaseUrl when not http", () => { @@ -198,15 +216,19 @@ describe("resolveImageWallpaperUrl", () => { ); }); - it("throws loudly when assetBaseUrl is empty (no silent fallback)", () => { + it("wraps AssetBaseUnavailableError in BackgroundLoadError when assetBaseUrl is empty", () => { vi.stubGlobal("window", { ...globalThis.window, location: { protocol: "file:" }, electronAPI: { assetBaseUrl: "" }, }); - expect(() => resolveImageWallpaperUrl("/wallpapers/wallpaper1.jpg")).toThrow( - AssetBaseUnavailableError, - ); + try { + resolveImageWallpaperUrl("/wallpapers/wallpaper1.jpg"); + expect.fail("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(BackgroundLoadError); + expect((err as BackgroundLoadError).cause).toBeInstanceOf(AssetBaseUnavailableError); + } }); }); @@ -229,6 +251,16 @@ describe("BackgroundLoadError", () => { expect(err.displayUrl).toBe("data:…"); }); + it("displayUrl returns sentinel for empty-basename URLs", () => { + const err = new BackgroundLoadError("file:///"); + expect(err.displayUrl).toBe("(unknown)"); + }); + + it("displayUrl returns sentinel for unparseable bare slash", () => { + const err = new BackgroundLoadError("/"); + expect(err.displayUrl).toBe("(unknown)"); + }); + it("preserves cause when provided", () => { const cause = new Error("inner"); const err = new BackgroundLoadError("file:///missing.jpg", cause); diff --git a/src/lib/wallpaper.ts b/src/lib/wallpaper.ts index c86ce43..34869d7 100644 --- a/src/lib/wallpaper.ts +++ b/src/lib/wallpaper.ts @@ -14,7 +14,7 @@ export type WallpaperClassification = | { kind: "gradient"; value: string } | { kind: "image"; path: string }; -const GRADIENT_RE = /^(linear|radial|conic)-gradient\(/; +const GRADIENT_RE = /^(repeating-)?(linear|radial|conic)-gradient\(/; const COLOR_FUNC_RE = /^(rgb|rgba|hsl|hsla|hwb|lab|lch|oklab|oklch|color)\(/; const IMAGE_URL_RE = /^(\/|https?:\/\/|file:\/\/|data:)/; @@ -53,7 +53,11 @@ export function resolveImageWallpaperUrl(imagePath: string): string { new Error(`Image wallpaper path must live under ${ALLOWED_IMAGE_PREFIX}`), ); } - return getAssetPath(withLeadingSlash.slice(1)); + try { + return getAssetPath(withLeadingSlash.slice(1)); + } catch (cause) { + throw new BackgroundLoadError(imagePath, cause); + } } export class BackgroundLoadError extends Error { @@ -79,9 +83,9 @@ function displayBasename(url: string): string { try { const parsed = new URL(url); const last = parsed.pathname.split("/").filter(Boolean).pop(); - return last ? decodeURIComponent(last) : url; + return last ? decodeURIComponent(last) : "(unknown)"; } catch { const last = url.split("/").filter(Boolean).pop(); - return last ?? url; + return last ?? "(unknown)"; } } diff --git a/src/vite-env.d.ts b/src/vite-env.d.ts index bdcb537..b7a0735 100644 --- a/src/vite-env.d.ts +++ b/src/vite-env.d.ts @@ -1,126 +1,2 @@ /// /// - -interface ProcessedDesktopSource { - id: string; - name: string; - display_id: string; - thumbnail: string | null; - appIcon: string | null; -} - -interface CursorTelemetryPoint { - timeMs: number; - cx: number; - cy: number; -} - -interface Window { - electronAPI: { - getSources: (opts: Electron.SourcesOptions) => Promise; - switchToEditor: () => Promise; - switchToHud: () => Promise; - startNewRecording: () => Promise<{ success: boolean; error?: string }>; - openSourceSelector: () => Promise; - selectSource: (source: ProcessedDesktopSource) => Promise; - getSelectedSource: () => Promise; - requestCameraAccess: () => Promise<{ - success: boolean; - granted: boolean; - status: string; - error?: string; - }>; - storeRecordedVideo: ( - videoData: ArrayBuffer, - fileName: string, - ) => Promise<{ - success: boolean; - path?: string; - session?: import("./lib/recordingSession").RecordingSession; - message?: string; - error?: string; - }>; - storeRecordedSession: ( - payload: import("./lib/recordingSession").StoreRecordedSessionInput, - ) => Promise<{ - success: boolean; - path?: string; - session?: import("./lib/recordingSession").RecordingSession; - message?: string; - error?: string; - }>; - getRecordedVideoPath: () => Promise<{ - success: boolean; - path?: string; - message?: string; - error?: string; - }>; - assetBaseUrl: string; - setRecordingState: (recording: boolean) => Promise; - getCursorTelemetry: (videoPath?: string) => Promise<{ - success: boolean; - samples: CursorTelemetryPoint[]; - message?: string; - error?: string; - }>; - onStopRecordingFromTray: (callback: () => void) => () => void; - openExternalUrl: (url: string) => Promise<{ success: boolean; error?: string }>; - saveExportedVideo: ( - videoData: ArrayBuffer, - fileName: string, - ) => Promise<{ - success: boolean; - path?: string; - message?: string; - canceled?: boolean; - }>; - openVideoFilePicker: () => Promise<{ success: boolean; path?: string; canceled?: boolean }>; - setCurrentVideoPath: (path: string) => Promise<{ success: boolean }>; - setCurrentRecordingSession: ( - session: import("./lib/recordingSession").RecordingSession | null, - ) => Promise<{ - success: boolean; - session?: import("./lib/recordingSession").RecordingSession; - }>; - getCurrentVideoPath: () => Promise<{ success: boolean; path?: string }>; - getCurrentRecordingSession: () => Promise<{ - success: boolean; - session?: import("./lib/recordingSession").RecordingSession; - }>; - clearCurrentVideoPath: () => Promise<{ success: boolean }>; - saveProjectFile: ( - projectData: unknown, - suggestedName?: string, - existingProjectPath?: string, - ) => Promise<{ - success: boolean; - path?: string; - message?: string; - canceled?: boolean; - error?: string; - }>; - loadProjectFile: () => Promise<{ - success: boolean; - path?: string; - project?: unknown; - message?: string; - canceled?: boolean; - error?: string; - }>; - loadCurrentProjectFile: () => Promise<{ - success: boolean; - path?: string; - project?: unknown; - message?: string; - canceled?: boolean; - error?: string; - }>; - onMenuLoadProject: (callback: () => void) => () => void; - onMenuSaveProject: (callback: () => void) => () => void; - onMenuSaveProjectAs: (callback: () => void) => () => void; - setMicrophoneExpanded: (expanded: boolean) => void; - setHasUnsavedChanges: (hasChanges: boolean) => void; - onRequestSaveBeforeClose: (callback: () => Promise | boolean) => () => void; - setLocale: (locale: string) => Promise; - }; -}