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
This commit is contained in:
jevb
2026-03-22 12:07:24 +01:00
parent 09fca14422
commit a653eb33ed
12 changed files with 83 additions and 26 deletions
@@ -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<string>("accentColor", "#5865f2"),
);
if (loadPref<boolean>("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<boolean>("syncOsMotion", false));
}
// ---------------------------------------------------------------------------
@@ -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);
}
@@ -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<boolean>("roleColors", true);
window.addEventListener("owncord:pref-change", ((e: CustomEvent<{ key: string }>) => {
if (e.detail.key === "roleColors") {
roleColorsEnabled = loadPref<boolean>("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<boolean>("roleColors", true)) {
if (!roleColorsEnabled) {
return "var(--role-member)";
}
switch (role) {
@@ -402,6 +402,11 @@ export function renderUrlEmbeds(content: string): DocumentFragment {
log.debug("renderUrlEmbeds", { urlCount: urls.length, urls });
const seen = new Set<string>();
// 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));
}
@@ -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<boolean>("developerMode", false);
window.addEventListener("owncord:pref-change", ((e: CustomEvent<{ key: string }>) => {
if (e.detail.key === "developerMode") {
developerModeEnabled = loadPref<boolean>("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);
}
@@ -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<ToggleItem> = [
label: "Sync with OS",
desc: "Automatically enable reduced motion based on your OS accessibility settings",
fallback: false,
sideEffect: (nowOn) => { syncOsMotionListener(nowOn); },
},
{
key: "largeFont",
@@ -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<typeof setTimeout> | 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();
};
@@ -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);
@@ -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);
@@ -33,6 +33,9 @@ export function loadPref<T>(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 } }));
}
// ---------------------------------------------------------------------------
+24
View File
@@ -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 });
}
+4
View File
@@ -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);