From a653eb33edb0c72b1b61f5bca49baa6bf93d8447 Mon Sep 17 00:00:00 2001 From: jevb Date: Sun, 22 Mar 2026 12:07:24 +0100 Subject: [PATCH] fix: address code review issues in settings and message rendering - Wire animateGifs pref to file attachments (observeMedia startFrozen) - Fix stale user closure in AccountTab edit form - Cache loadPref reads in hot render paths with custom event invalidation - Extract syncOsMotion listener to lib/os-motion.ts with AbortController cleanup - Add sideEffect to syncOsMotion toggle for in-session activation - Use static import for invoke in AdvancedTab, add error logging - Fix password success timer race in AccountTab - Extract keybind section header to CSS class - Handle clipboard.writeText rejection in Copy ID button - Label hardware acceleration as requiring restart --- .../src/components/SettingsOverlay.ts | 9 ++----- .../components/message-list/attachments.ts | 5 ++-- .../src/components/message-list/formatting.ts | 10 +++++++- .../src/components/message-list/media.ts | 11 ++++++--- .../src/components/message-list/renderers.ts | 14 +++++++++-- .../components/settings/AccessibilityTab.ts | 2 ++ .../src/components/settings/AccountTab.ts | 10 ++++++-- .../src/components/settings/AdvancedTab.ts | 11 ++++----- .../src/components/settings/KeybindsTab.ts | 6 ++--- .../src/components/settings/helpers.ts | 3 +++ Client/tauri-client/src/lib/os-motion.ts | 24 +++++++++++++++++++ Client/tauri-client/src/styles/app.css | 4 ++++ 12 files changed, 83 insertions(+), 26 deletions(-) create mode 100644 Client/tauri-client/src/lib/os-motion.ts diff --git a/Client/tauri-client/src/components/SettingsOverlay.ts b/Client/tauri-client/src/components/SettingsOverlay.ts index e213ed1d..909f3bd9 100644 --- a/Client/tauri-client/src/components/SettingsOverlay.ts +++ b/Client/tauri-client/src/components/SettingsOverlay.ts @@ -13,6 +13,7 @@ import { uiStore } from "@stores/ui.store"; import { authStore } from "@stores/auth.store"; import { loadPref, applyTheme } from "./settings/helpers"; import type { ThemeName } from "./settings/helpers"; +import { syncOsMotionListener } from "@lib/os-motion"; import { buildAccountTab } from "./settings/AccountTab"; import { buildAppearanceTab } from "./settings/AppearanceTab"; import { buildNotificationsTab } from "./settings/NotificationsTab"; @@ -75,13 +76,7 @@ export function applyStoredAppearance(): void { loadPref("accentColor", "#5865f2"), ); - if (loadPref("syncOsMotion", false)) { - const mq = window.matchMedia("(prefers-reduced-motion: reduce)"); - document.documentElement.classList.toggle("reduced-motion", mq.matches); - mq.addEventListener("change", (e: MediaQueryListEvent) => { - document.documentElement.classList.toggle("reduced-motion", e.matches); - }); - } + syncOsMotionListener(loadPref("syncOsMotion", false)); } // --------------------------------------------------------------------------- diff --git a/Client/tauri-client/src/components/message-list/attachments.ts b/Client/tauri-client/src/components/message-list/attachments.ts index aff460ac..0bd79a7c 100644 --- a/Client/tauri-client/src/components/message-list/attachments.ts +++ b/Client/tauri-client/src/components/message-list/attachments.ts @@ -10,6 +10,7 @@ import { } from "@lib/dom"; import { createIcon } from "@lib/icons"; import { observeMedia } from "@lib/media-visibility"; +import { loadPref } from "@components/settings/helpers"; import { fetch as tauriFetch } from "@tauri-apps/plugin-http"; import { save } from "@tauri-apps/plugin-dialog"; import { writeFile } from "@tauri-apps/plugin-fs"; @@ -231,7 +232,7 @@ export function renderAttachment(att: Attachment): HTMLDivElement { attachLightbox(img); img.addEventListener("load", () => { clearReservation(); - if (isGif) observeMedia(img, cached, wrap); + if (isGif) observeMedia(img, cached, wrap, !loadPref("animateGifs", true)); }, { once: true }); wrap.appendChild(img); } else { @@ -248,7 +249,7 @@ export function renderAttachment(att: Attachment): HTMLDivElement { attachLightbox(img); img.addEventListener("load", () => { clearReservation(); - if (isGif) observeMedia(img, dataUrl, wrap); + if (isGif) observeMedia(img, dataUrl, wrap, !loadPref("animateGifs", true)); }, { once: true }); placeholder.replaceWith(img); } diff --git a/Client/tauri-client/src/components/message-list/formatting.ts b/Client/tauri-client/src/components/message-list/formatting.ts index 0fed7133..a3b4b9d0 100644 --- a/Client/tauri-client/src/components/message-list/formatting.ts +++ b/Client/tauri-client/src/components/message-list/formatting.ts @@ -84,12 +84,20 @@ export function shouldGroup(prev: Message, curr: Message): boolean { // -- Role helpers ------------------------------------------------------------- +/** Cached value of the roleColors preference. Invalidated on pref change. */ +let roleColorsEnabled = loadPref("roleColors", true); +window.addEventListener("owncord:pref-change", ((e: CustomEvent<{ key: string }>) => { + if (e.detail.key === "roleColors") { + roleColorsEnabled = loadPref("roleColors", true); + } +}) as EventListener); + export function getUserRole(userId: number): string { return membersStore.getState().members.get(userId)?.role ?? "member"; } export function roleColorVar(role: string): string { - if (!loadPref("roleColors", true)) { + if (!roleColorsEnabled) { return "var(--role-member)"; } switch (role) { diff --git a/Client/tauri-client/src/components/message-list/media.ts b/Client/tauri-client/src/components/message-list/media.ts index 8a7d4ca6..fffafd43 100644 --- a/Client/tauri-client/src/components/message-list/media.ts +++ b/Client/tauri-client/src/components/message-list/media.ts @@ -402,6 +402,11 @@ export function renderUrlEmbeds(content: string): DocumentFragment { log.debug("renderUrlEmbeds", { urlCount: urls.length, urls }); const seen = new Set(); + // Read preferences once before the loop to avoid per-URL localStorage reads + const showEmbeds = loadPref("showEmbeds", true); + const inlineMedia = loadPref("inlineMedia", true); + const showLinkPreviews = loadPref("showLinkPreviews", true); + for (const url of urls) { if (seen.has(url)) continue; seen.add(url); @@ -409,7 +414,7 @@ export function renderUrlEmbeds(content: string): DocumentFragment { // YouTube embed const ytId = extractYouTubeId(url); if (ytId !== null) { - if (!loadPref("showEmbeds", true)) continue; + if (!showEmbeds) continue; fragment.appendChild(renderYouTubeEmbed(ytId, url)); continue; } @@ -419,14 +424,14 @@ export function renderUrlEmbeds(content: string): DocumentFragment { const isSafe = isSafeUrl(url); log.debug("URL classification", { url: url.slice(0, 80), isDirect, isSafe, isGif: isGifUrl(url) }); if (isDirect && isSafe) { - if (!loadPref("inlineMedia", true)) continue; + if (!inlineMedia) continue; fragment.appendChild(renderInlineImage(url)); continue; } // Generic URL preview (compact link card) if (isSafe) { - if (!loadPref("showLinkPreviews", true)) continue; + if (!showLinkPreviews) continue; log.debug("Falling through to generic link preview", { url: url.slice(0, 80) }); fragment.appendChild(renderGenericLinkPreview(url)); } diff --git a/Client/tauri-client/src/components/message-list/renderers.ts b/Client/tauri-client/src/components/message-list/renderers.ts index 3554290a..9d3ee498 100644 --- a/Client/tauri-client/src/components/message-list/renderers.ts +++ b/Client/tauri-client/src/components/message-list/renderers.ts @@ -14,6 +14,14 @@ import { loadPref } from "@components/settings/helpers"; import type { Message } from "@stores/messages.store"; import type { MessageListOptions } from "../MessageList"; +/** Cached value of the developerMode preference. Invalidated on pref change. */ +let developerModeEnabled = loadPref("developerMode", false); +window.addEventListener("owncord:pref-change", ((e: CustomEvent<{ key: string }>) => { + if (e.detail.key === "developerMode") { + developerModeEnabled = loadPref("developerMode", false); + } +}) as EventListener); + // -- Re-exports (preserve all existing public API) ---------------------------- export { @@ -241,11 +249,13 @@ export function renderMessage( actionsBar.appendChild(deleteBtn); } - if (loadPref("developerMode", false)) { + if (developerModeEnabled) { const copyIdBtn = createElement("button", { "data-testid": `msg-copy-id-${msg.id}` }); copyIdBtn.appendChild(createIcon("hash", 16)); copyIdBtn.title = "Copy ID"; - copyIdBtn.addEventListener("click", () => navigator.clipboard.writeText(String(msg.id)), { signal }); + copyIdBtn.addEventListener("click", () => { + void navigator.clipboard.writeText(String(msg.id)).catch(() => { /* clipboard unavailable */ }); + }, { signal }); actionsBar.appendChild(copyIdBtn); } diff --git a/Client/tauri-client/src/components/settings/AccessibilityTab.ts b/Client/tauri-client/src/components/settings/AccessibilityTab.ts index 49b80500..3daddf70 100644 --- a/Client/tauri-client/src/components/settings/AccessibilityTab.ts +++ b/Client/tauri-client/src/components/settings/AccessibilityTab.ts @@ -4,6 +4,7 @@ import { createElement, appendChildren } from "@lib/dom"; import { loadPref, savePref, createToggle } from "./helpers"; +import { syncOsMotionListener } from "@lib/os-motion"; type ToggleItem = { readonly key: string; @@ -43,6 +44,7 @@ const TOGGLES: ReadonlyArray = [ label: "Sync with OS", desc: "Automatically enable reduced motion based on your OS accessibility settings", fallback: false, + sideEffect: (nowOn) => { syncOsMotionListener(nowOn); }, }, { key: "largeFont", diff --git a/Client/tauri-client/src/components/settings/AccountTab.ts b/Client/tauri-client/src/components/settings/AccountTab.ts index 212e9bea..1c8b5870 100644 --- a/Client/tauri-client/src/components/settings/AccountTab.ts +++ b/Client/tauri-client/src/components/settings/AccountTab.ts @@ -87,6 +87,7 @@ function buildPasswordSection( }); const pwError = createElement("div", { style: "color:var(--red);font-size:13px;margin-bottom:8px" }); const pwBtn = createElement("button", { class: "ac-btn" }, "Change Password"); + let pwSuccessTimer: ReturnType | null = null; pwBtn.addEventListener("click", () => { const oldVal = oldPw.value; @@ -106,9 +107,14 @@ function buildPasswordSection( oldPw.value = ""; newPw.value = ""; confirmPw.value = ""; + if (pwSuccessTimer !== null) clearTimeout(pwSuccessTimer); pwError.style.color = "var(--green)"; setText(pwError, "Password changed successfully."); - setTimeout(() => { setText(pwError, ""); pwError.style.color = "var(--red)"; }, 3000); + pwSuccessTimer = setTimeout(() => { + setText(pwError, ""); + pwError.style.color = "var(--red)"; + pwSuccessTimer = null; + }, 3000); }).catch((err: unknown) => { setText(pwError, err instanceof Error ? err.message : "Failed to change password."); }); @@ -217,7 +223,7 @@ export function buildAccountTab( const openEditForm = () => { editForm.style.display = "flex"; - editInput.value = user?.username ?? ""; + editInput.value = authStore.getState().user?.username ?? ""; editInput.focus(); }; diff --git a/Client/tauri-client/src/components/settings/AdvancedTab.ts b/Client/tauri-client/src/components/settings/AdvancedTab.ts index 2d45db72..a3176425 100644 --- a/Client/tauri-client/src/components/settings/AdvancedTab.ts +++ b/Client/tauri-client/src/components/settings/AdvancedTab.ts @@ -3,6 +3,7 @@ */ import { createElement, appendChildren } from "@lib/dom"; +import { invoke } from "@tauri-apps/api/core"; import { loadPref, savePref, createToggle } from "./helpers"; export function buildAdvancedTab(signal: AbortSignal): HTMLDivElement { @@ -20,7 +21,7 @@ export function buildAdvancedTab(signal: AbortSignal): HTMLDivElement { { key: "hardwareAcceleration", label: "Hardware Acceleration", - desc: "Use GPU for rendering. Disable if experiencing graphical issues", + desc: "Use GPU for rendering. Requires restart to take effect", fallback: true, }, ]; @@ -61,11 +62,9 @@ export function buildAdvancedTab(signal: AbortSignal): HTMLDivElement { const devtoolsBtn = createElement("button", { class: "ac-btn" }, "Open DevTools"); devtoolsBtn.addEventListener("click", () => { - void import("@tauri-apps/api/core") - .then((mod) => mod.invoke("open_devtools")) - .catch(() => { - // DevTools not available in this build - }); + void invoke("open_devtools").catch((err: unknown) => { + console.warn("DevTools not available:", err); + }); }, { signal }); appendChildren(devtoolsRow, devtoolsInfo, devtoolsBtn); diff --git a/Client/tauri-client/src/components/settings/KeybindsTab.ts b/Client/tauri-client/src/components/settings/KeybindsTab.ts index 537b8bfc..f8451307 100644 --- a/Client/tauri-client/src/components/settings/KeybindsTab.ts +++ b/Client/tauri-client/src/components/settings/KeybindsTab.ts @@ -78,7 +78,7 @@ export function buildKeybindsTab(signal: AbortSignal): HTMLDivElement { section.appendChild(createElement("div", { class: "settings-separator" })); const navHeader = createElement("div", { - style: "font-size: 12px; font-weight: 600; color: var(--text-muted); text-transform: uppercase; letter-spacing: 0.5px; margin: 4px 0 8px 0;", + class: "keybind-section-header", }, "Navigation"); section.appendChild(navHeader); @@ -100,7 +100,7 @@ export function buildKeybindsTab(signal: AbortSignal): HTMLDivElement { section.appendChild(createElement("div", { class: "settings-separator" })); const commHeader = createElement("div", { - style: "font-size: 12px; font-weight: 600; color: var(--text-muted); text-transform: uppercase; letter-spacing: 0.5px; margin: 4px 0 8px 0;", + class: "keybind-section-header", }, "Communication"); section.appendChild(commHeader); @@ -122,7 +122,7 @@ export function buildKeybindsTab(signal: AbortSignal): HTMLDivElement { section.appendChild(createElement("div", { class: "settings-separator" })); const msgHeader = createElement("div", { - style: "font-size: 12px; font-weight: 600; color: var(--text-muted); text-transform: uppercase; letter-spacing: 0.5px; margin: 4px 0 8px 0;", + class: "keybind-section-header", }, "Messages"); section.appendChild(msgHeader); diff --git a/Client/tauri-client/src/components/settings/helpers.ts b/Client/tauri-client/src/components/settings/helpers.ts index e3bf6c98..852c2377 100644 --- a/Client/tauri-client/src/components/settings/helpers.ts +++ b/Client/tauri-client/src/components/settings/helpers.ts @@ -33,6 +33,9 @@ export function loadPref(key: string, fallback: T): T { export function savePref(key: string, value: unknown): void { localStorage.setItem(STORAGE_PREFIX + key, JSON.stringify(value)); + // Dispatch a custom event so same-window listeners can invalidate caches. + // The native `storage` event only fires for cross-tab changes. + window.dispatchEvent(new CustomEvent("owncord:pref-change", { detail: { key } })); } // --------------------------------------------------------------------------- diff --git a/Client/tauri-client/src/lib/os-motion.ts b/Client/tauri-client/src/lib/os-motion.ts new file mode 100644 index 00000000..0d11bd62 --- /dev/null +++ b/Client/tauri-client/src/lib/os-motion.ts @@ -0,0 +1,24 @@ +/** + * OS reduced-motion sync — managed listener with safe re-registration. + * Extracted to its own module to avoid circular dependencies between + * SettingsOverlay and AccessibilityTab. + */ + +let ac: AbortController | null = null; + +/** Enable or disable the OS reduced-motion sync listener. Safe to call multiple times. */ +export function syncOsMotionListener(enabled: boolean): void { + // Tear down any previous listener + if (ac !== null) { + ac.abort(); + ac = null; + } + if (!enabled) return; + + ac = new AbortController(); + const mq = window.matchMedia("(prefers-reduced-motion: reduce)"); + document.documentElement.classList.toggle("reduced-motion", mq.matches); + mq.addEventListener("change", (e: MediaQueryListEvent) => { + document.documentElement.classList.toggle("reduced-motion", e.matches); + }, { signal: ac.signal }); +} diff --git a/Client/tauri-client/src/styles/app.css b/Client/tauri-client/src/styles/app.css index 2854a963..642bd857 100644 --- a/Client/tauri-client/src/styles/app.css +++ b/Client/tauri-client/src/styles/app.css @@ -1175,6 +1175,10 @@ .accent-hex-prefix { font-size: 14px; color: var(--text-muted); user-select: none; } +.keybind-section-header { + font-size: 12px; font-weight: 600; color: var(--text-muted); + text-transform: uppercase; letter-spacing: 0.5px; margin: 4px 0 8px 0; +} .keybind-row { display: flex; align-items: center; justify-content: space-between; padding: 12px 0; border-bottom: 1px solid var(--border);