From 29b44f1b4c373c3b6a1c27ed6cc56a09d17e223c Mon Sep 17 00:00:00 2001 From: teernisse Date: Thu, 26 Feb 2026 10:12:20 -0500 Subject: [PATCH] fix: patch memory leaks in event hooks and strengthen type guard Fix three bugs found during code review: 1. useTauriEvent/useTauriEvents: If the component unmounts before the async listen() promise resolves, the unlisten function was lost, leaking the event subscription. Added a cancelled flag to call unlisten immediately when the promise resolves after cleanup. 2. useTauriEvents: The handlers object was used directly as a useEffect dependency, causing re-subscription on every render when callers pass an inline object literal. Replaced with a useRef for handler stability and a derived eventNames string as the dependency. 3. isMcError type guard: Only checked property existence via 'in' operator, not property types. An object with wrong-typed properties (e.g. code: 42) would pass the guard. Now validates that code and message are strings and recoverable is boolean. 4. AppShell global shortcut listener: Same race condition as (1), plus missing .catch() on the listen promise could produce unhandled rejections. Co-Authored-By: Claude Opus 4.5 --- src/components/AppShell.tsx | 22 +++++++++-- src/hooks/useTauriEvents.ts | 77 ++++++++++++++++++++----------------- src/lib/types.ts | 14 ++++--- 3 files changed, 69 insertions(+), 44 deletions(-) diff --git a/src/components/AppShell.tsx b/src/components/AppShell.tsx index 43f2658..8c102ba 100644 --- a/src/components/AppShell.tsx +++ b/src/components/AppShell.tsx @@ -38,14 +38,30 @@ export function AppShell(): React.ReactElement { // Listen for global shortcut events from the Rust backend useEffect(() => { - const unlisten = listen("global-shortcut-triggered", (event) => { + let cancelled = false; + let unlisten: (() => void) | undefined; + + listen("global-shortcut-triggered", (event) => { if (event.payload === "quick-capture") { useCaptureStore.getState().open(); } - }); + }) + .then((fn) => { + if (cancelled) { + fn(); + } else { + unlisten = fn; + } + }) + .catch((err: unknown) => { + console.warn("[AppShell] Failed to subscribe to global shortcut:", err); + }); return () => { - unlisten.then((fn) => fn()); + cancelled = true; + if (unlisten) { + unlisten(); + } }; }, []); diff --git a/src/hooks/useTauriEvents.ts b/src/hooks/useTauriEvents.ts index 6abe63f..1781d14 100644 --- a/src/hooks/useTauriEvents.ts +++ b/src/hooks/useTauriEvents.ts @@ -5,7 +5,7 @@ * Used for file watcher triggers, sync status, error notifications. */ -import { useEffect, useCallback } from "react"; +import { useEffect, useRef } from "react"; import { listen, type UnlistenFn } from "@tauri-apps/api/event"; /** Event types emitted by the Rust backend */ @@ -26,79 +26,84 @@ export interface TauriEventPayloads { /** * Subscribe to a Tauri event with automatic cleanup. * - * @param eventName The event to listen for - * @param handler Callback when event is received - * - * @example - * ```tsx - * useTauriEvent("lore-data-changed", () => { - * refetch(); - * }); - * ``` + * Uses a ref for the handler so that changing the callback does not + * cause re-subscription. Handles the race where the component unmounts + * before the async listen() promise resolves. */ export function useTauriEvent( eventName: T, handler: (payload: TauriEventPayloads[T]) => void ): void { - // Memoize handler to avoid re-subscribing on every render - const stableHandler = useCallback(handler, [handler]); + const handlerRef = useRef(handler); + handlerRef.current = handler; useEffect(() => { + let cancelled = false; let unlisten: UnlistenFn | undefined; - // Subscribe to the event listen(eventName, (event) => { - stableHandler(event.payload); + handlerRef.current(event.payload); }).then((fn) => { - unlisten = fn; + if (cancelled) { + fn(); + } else { + unlisten = fn; + } }); - // Cleanup on unmount return () => { + cancelled = true; if (unlisten) { unlisten(); } }; - }, [eventName, stableHandler]); + }, [eventName]); } /** * Subscribe to multiple Tauri events. * - * @param handlers Map of event names to handlers - * - * @example - * ```tsx - * useTauriEvents({ - * "lore-data-changed": () => refetch(), - * "sync-status": (status) => setStatus(status), - * }); - * ``` + * Uses a ref for the handlers object so that identity changes do not + * cause re-subscription. Only re-subscribes when the set of event + * names changes. */ export function useTauriEvents( handlers: Partial<{ [K in TauriEventType]: (payload: TauriEventPayloads[K]) => void; }> ): void { + const handlersRef = useRef(handlers); + handlersRef.current = handlers; + + const eventNames = Object.keys(handlers).sort().join(","); + useEffect(() => { + let cancelled = false; const unlisteners: UnlistenFn[] = []; - // Subscribe to each event - for (const [eventName, handler] of Object.entries(handlers)) { - if (handler) { - listen(eventName, (event) => { - (handler as (p: unknown) => void)(event.payload); - }).then((unlisten) => { + const entries = eventNames.split(",").filter(Boolean); + for (const eventName of entries) { + listen(eventName, (event) => { + const currentHandler = handlersRef.current[eventName as TauriEventType]; + if (currentHandler) { + // Safe cast: handler was registered for this event name, but TS + // cannot narrow the key-value type relationship at runtime + (currentHandler as (p: unknown) => void)(event.payload); + } + }).then((unlisten) => { + if (cancelled) { + unlisten(); + } else { unlisteners.push(unlisten); - }); - } + } + }); } - // Cleanup all subscriptions on unmount return () => { + cancelled = true; for (const unlisten of unlisteners) { unlisten(); } }; - }, [handlers]); + }, [eventNames]); } diff --git a/src/lib/types.ts b/src/lib/types.ts index cfda6d4..40b1927 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -63,12 +63,12 @@ export interface McError { /** Type guard to check if an error is a structured McError */ export function isMcError(err: unknown): err is McError { + if (typeof err !== "object" || err === null) return false; + const obj = err as Record; return ( - typeof err === "object" && - err !== null && - "code" in err && - "message" in err && - "recoverable" in err + typeof obj.code === "string" && + typeof obj.message === "string" && + typeof obj.recoverable === "boolean" ); } @@ -156,6 +156,10 @@ export function computeStaleness(updatedAt: string | null): Staleness { if (!updatedAt) return "normal"; const ageMs = Date.now() - new Date(updatedAt).getTime(); + + // Guard against invalid date strings (NaN propagates through arithmetic) + if (Number.isNaN(ageMs)) return "normal"; + const ageDays = ageMs / (1000 * 60 * 60 * 24); if (ageDays < 1) return "fresh";