Merge pull request #424 from Mng-dev-ai/refactor/useeffect-discipline

Reduce useEffect usage with useMountEffect and ref-based resets

Author: Mng-dev-ai

CLAUDE.md

@@ -114,6 +114,12 @@
 - The `components/chat/tools/` directory is exclusively for tool components (one per tool type) — helper modals, dialogs, and detail views triggered by tools belong in `components/chat/` or a relevant feature folder, not in `tools/`
 - Shared UI components used by 2+ feature areas belong in `components/ui/shared/` — do not place them loose in a feature folder just because the first consumer lives there
 
+### useEffect Discipline
+- Never call `useEffect` directly for mount-only effects — use `useMountEffect()` from `hooks/useMountEffect.ts` (`useEffect(fn, [])` wrapper) to make intent explicit and enable lint enforcement
+- Never use `useEffect` to derive state from other state or props — if state is always computable from existing values, use inline computation or `useMemo`; `useEffect(() => setX(f(y)), [y])` causes an unnecessary extra render cycle
+- When state must reset on a prop/ID change, use a ref-based render check instead of `useEffect` — pattern: `const prevRef = useRef(prop); if (prevRef.current !== prop) { prevRef.current = prop; setState(initial); }` — this runs synchronously during render and avoids the effect-induced double render
+- Distinguish "derived state" from "form state initialization" — if local state is a copy of server data that the user then edits independently (e.g., secrets form, settings form), syncing it via `useEffect` on query data change is the correct pattern; do not convert these to inline derivation, as the local state intentionally diverges from the source
+
 ### Component Variants
 - Create explicit variant components instead of one component with many boolean modes (e.g., `ThreadComposer`, `EditComposer` instead of `<Composer isThread isEditing />`)
 - Use `children` for composing static structure; use render props only when the parent needs to pass data back to the child (e.g., `renderItem` in lists)
@@ -139,6 +145,7 @@
 - Do not wrap trivial expressions in `useMemo` (e.g., `useMemo(() => x || [], [x])`) — use direct expressions (`x ?? []`)
 - Hoist regex patterns to module-level constants — never create RegExp inside loops or frequently-called functions
 - Prefer single-pass iteration (`.reduce()`) over chained `.filter().map()` in render paths
+- Keep `useEffect` for external system subscriptions and DOM side effects — keyboard shortcuts, resize observers, WebSocket lifecycle, scroll-into-view, and focus management require post-render timing and cleanup; do not convert these to ref-based render checks
 
 ### Async Patterns
 - Use `Promise.all()` for independent async operations (e.g., multiple `queryClient.invalidateQueries()` calls)

frontend/src/App.tsx

@@ -1,5 +1,6 @@
 import { BrowserRouter, Routes, Route } from 'react-router-dom';
 import { useEffect, useMemo, useState, Suspense, lazy } from 'react';
+import { useMountEffect } from '@/hooks/useMountEffect';
 import { Layout } from '@/components/layout/Layout';
 import { Toaster } from 'react-hot-toast';
 import toast from 'react-hot-toast';
@@ -184,7 +185,7 @@ export default function App() {
 
   useGlobalStream({ enabled: authHydrated && desktopReady });
 
-  useEffect(() => {
+  useMountEffect(() => {
     let cancelled = false;
 
     authStorage
@@ -201,7 +202,7 @@ export default function App() {
     return () => {
       cancelled = true;
     };
-  }, []);
+  });
 
   useEffect(() => {
     document.body.classList.remove('light', 'dark');
@@ -213,7 +214,7 @@ export default function App() {
     }
   }, [resolvedTheme]);
 
-  useEffect(() => {
+  useMountEffect(() => {
     if (!isTauri()) return;
 
     let cancelled = false;
@@ -243,18 +244,18 @@ export default function App() {
     return () => {
       cancelled = true;
     };
-  }, []);
+  });
 
-  useEffect(() => {
+  useMountEffect(() => {
     if (import.meta.env.DEV || !isTauri()) return;
 
     checkDesktopUpdate().catch((error) => {
       console.error('Desktop updater check failed:', error);
     });
-  }, []);
+  });
 
   // Open external links in the system browser — Tauri doesn't handle target="_blank" natively
-  useEffect(() => {
+  useMountEffect(() => {
     if (!isTauri()) return;
 
     let openUrl: ((url: string) => Promise<void>) | null = null;
@@ -276,7 +277,7 @@ export default function App() {
 
     document.addEventListener('click', handler);
     return () => document.removeEventListener('click', handler);
-  }, []);
+  });
 
   if (desktopError) {
     return (

frontend/src/components/chat/chat-window/Chat.tsx

@@ -156,17 +156,19 @@ export const Chat = memo(function Chat() {
   const lastScrollTopRef = useRef<number | null>(null);
   const isAtBottomRef = useRef(true);
   const prevScrollHeightRef = useRef<number | null>(null);
+  const prevChatIdForScrollRef = useRef(chatId);
 
   const [showScrollButton, setShowScrollButton] = useState(false);
 
-  useEffect(() => {
+  if (prevChatIdForScrollRef.current !== chatId) {
+    prevChatIdForScrollRef.current = chatId;
     hasInitializedToBottomRef.current = false;
     topPaginationArmedRef.current = false;
     lastScrollTopRef.current = null;
     isAtBottomRef.current = true;
     prevScrollHeightRef.current = null;
     setShowScrollButton(false);
-  }, [chatId]);
+  }
 
   const fetchNextPageRef = useRef(fetchNextPage);
   const hasNextPageRef = useRef(hasNextPage);

frontend/src/components/chat/chat-window/ChatSessionOrchestrator.tsx

@@ -177,9 +177,11 @@ export function ChatSessionOrchestrator({
     hasFetchedMessages,
   ]);
 
-  useEffect(() => {
+  const prevChatIdForPromptRef = useRef(chatId);
+  if (prevChatIdForPromptRef.current !== chatId) {
+    prevChatIdForPromptRef.current = chatId;
     setInitialPromptSent(false);
-  }, [chatId, setInitialPromptSent]);
+  }
 
   const chatSessionState = useMemo<ChatSessionState>(
     () => ({

frontend/src/components/chat/chat-window/ThinkingIndicator.tsx

@@ -1,16 +1,17 @@
-import { memo, useState, useEffect, useRef } from 'react';
+import { memo, useState, useRef } from 'react';
+import { useMountEffect } from '@/hooks/useMountEffect';
 
 export const ThinkingIndicator = memo(function ThinkingIndicator() {
   const [elapsed, setElapsed] = useState(0);
   const startTime = useRef(Date.now());
 
-  useEffect(() => {
+  useMountEffect(() => {
     const interval = setInterval(() => {
       setElapsed(Math.floor((Date.now() - startTime.current) / 1000));
     }, 1000);
 
     return () => clearInterval(interval);
-  }, []);
+  });
 
   return (
     <div className="animate-fade-in px-4 pb-1.5 sm:px-6 sm:pb-2">

frontend/src/components/chat/message-input/Textarea.tsx

@@ -6,6 +6,7 @@ import {
   type CSSProperties,
   type Ref,
 } from 'react';
+import { useMountEffect } from '@/hooks/useMountEffect';
 import { useIsMobile } from '@/hooks/useIsMobile';
 
 const THIN_SCROLLBAR_STYLE: CSSProperties = { scrollbarWidth: 'thin' };
@@ -55,13 +56,13 @@ export function Textarea({
     }
   }, [isLoading, isMobile, textareaRef]);
 
-  useEffect(() => {
+  useMountEffect(() => {
     return () => {
       if (debounceTimerRef.current) {
         clearTimeout(debounceTimerRef.current);
       }
     };
-  }, []);
+  });
 
   const debouncedCursorChange = useCallback(
     (position: number) => {

frontend/src/components/chat/tools/AgentTool.tsx

@@ -1,4 +1,4 @@
-import React, { use, useMemo, useState, useEffect, lazy, Suspense } from 'react';
+import React, { use, useMemo, useState, useRef, lazy, Suspense } from 'react';
 import { Bot, Maximize2 } from 'lucide-react';
 import type { ToolAggregate } from '@/types/tools.types';
 import { ToolCard } from './common/ToolCard';
@@ -18,15 +18,17 @@ export const AgentTool: React.FC<AgentToolProps> = ({ tool }) => {
   const [resultExpanded, setResultExpanded] = useState(false);
   const [toolsExpanded, setToolsExpanded] = useState(false);
   const [modalOpen, setModalOpen] = useState(false);
+  const prevToolIdRef = useRef(tool.id);
 
   const siblingAgents = use(AgentToolsContext);
 
-  useEffect(() => {
+  if (prevToolIdRef.current !== tool.id) {
+    prevToolIdRef.current = tool.id;
     setPromptExpanded(false);
     setResultExpanded(false);
     setToolsExpanded(false);
     setModalOpen(false);
-  }, [tool.id]);
+  }
 
   const prompt = tool.input?.prompt as string | undefined;
   const description = tool.input?.description as string | undefined;

frontend/src/components/editor/editor-view/View.tsx

@@ -31,7 +31,6 @@ export const View = memo(function View({
   const previousFileRef = useRef<FileStructure | null>(null);
   const [showPreview, setShowPreview] = useState(false);
   const [currentContent, setCurrentContent] = useState('');
-  const [error, setError] = useState<string | null>(null);
   const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false);
   const editorRef = useRef<monaco.editor.IStandaloneCodeEditor | null>(null);
   const monacoRef = useRef<typeof monaco | null>(null);
@@ -76,17 +75,11 @@ export const View = memo(function View({
     }
   }, [selectedFile, fileContentData]);
 
-  useEffect(() => {
-    if (fileContentError) {
-      const errorMessage =
-        fileContentError instanceof Error
-          ? fileContentError.message
-          : 'Failed to load file content';
-      setError(errorMessage);
-    } else {
-      setError(null);
-    }
-  }, [fileContentError]);
+  const error = fileContentError
+    ? fileContentError instanceof Error
+      ? fileContentError.message
+      : 'Failed to load file content'
+    : null;
 
   useEffect(() => {
     if (!selectedFile) {
@@ -154,21 +147,25 @@ export const View = memo(function View({
   );
 
   const [isPreviewFullscreen, setIsPreviewFullscreen] = useState(false);
+  const prevShowPreviewRef = useRef(showPreview);
+  const prevFilePathRef = useRef(selectedFile?.path);
+
+  // Reset fullscreen when preview is hidden or file changes
+  if (
+    prevShowPreviewRef.current !== showPreview ||
+    prevFilePathRef.current !== selectedFile?.path
+  ) {
+    prevShowPreviewRef.current = showPreview;
+    prevFilePathRef.current = selectedFile?.path;
+    if (isPreviewFullscreen) {
+      setIsPreviewFullscreen(false);
+    }
+  }
 
   const handleTogglePreviewFullscreen = useCallback(() => {
     setIsPreviewFullscreen((prev) => !prev);
   }, []);
 
-  useEffect(() => {
-    if (!showPreview) {
-      setIsPreviewFullscreen(false);
-    }
-  }, [showPreview]);
-
-  useEffect(() => {
-    setIsPreviewFullscreen(false);
-  }, [selectedFile?.path]);
-
   useEffect(() => {
     if (!isPreviewFullscreen) return;
 

frontend/src/components/editor/file-preview/PDFPreview.tsx

@@ -1,4 +1,4 @@
-import { memo, useMemo, useState, useEffect } from 'react';
+import { memo, useMemo, useEffect, useState, useRef } from 'react';
 import { logger } from '@/utils/logger';
 import { base64ToUint8Array } from '@/utils/base64';
 import type { FileStructure } from '@/types/file-system.types';
@@ -17,52 +17,35 @@ export const PDFPreview = memo(function PDFPreview({
   isFullscreen = false,
   onToggleFullscreen,
 }: PDFPreviewProps) {
-  const [pdfUrl, setPdfUrl] = useState<string | null>(null);
-  const [isLoading, setIsLoading] = useState(true);
-  const [error, setError] = useState(false);
   const fileName = getDisplayFileName(file, 'document.pdf');
+  const [iframeError, setIframeError] = useState(false);
 
-  const blobUrl = useMemo(() => {
+  const pdfUrl = useMemo(() => {
     if (!file.content || !isValidBase64(file.content)) return null;
 
     try {
       const bytes = base64ToUint8Array(file.content);
       const blob = new Blob([bytes as unknown as BlobPart], { type: 'application/pdf' });
       return URL.createObjectURL(blob);
-    } catch (error) {
-      logger.error('PDF preview load failed', 'PDFPreview', error);
+    } catch (err) {
+      logger.error('PDF preview load failed', 'PDFPreview', err);
       return null;
     }
   }, [file.content]);
 
-  useEffect(() => {
-    if (blobUrl) {
-      setPdfUrl(blobUrl);
-      setError(false);
-    } else {
-      setError(true);
-    }
-    setIsLoading(false);
+  const prevPdfUrlRef = useRef(pdfUrl);
+  if (prevPdfUrlRef.current !== pdfUrl) {
+    prevPdfUrlRef.current = pdfUrl;
+    setIframeError(false);
+  }
 
+  useEffect(() => {
     return () => {
-      if (blobUrl) {
-        URL.revokeObjectURL(blobUrl);
-      }
+      if (pdfUrl) URL.revokeObjectURL(pdfUrl);
     };
-  }, [blobUrl]);
-
-  if (isLoading) {
-    return (
-      <PreviewEmptyState
-        fileName={fileName}
-        message="Loading PDF..."
-        isFullscreen={isFullscreen}
-        onToggleFullscreen={onToggleFullscreen}
-      />
-    );
-  }
+  }, [pdfUrl]);
 
-  if (error || !pdfUrl) {
+  if (!pdfUrl || iframeError) {
     return (
       <PreviewEmptyState
         fileName={fileName}
@@ -84,7 +67,7 @@ export const PDFPreview = memo(function PDFPreview({
         src={pdfUrl}
         className="h-full w-full border-0"
         title={fileName}
-        onError={() => setError(true)}
+        onError={() => setIframeError(true)}
       />
     </PreviewContainer>
   );

frontend/src/components/editor/file-tree/SearchInput.tsx

@@ -1,4 +1,5 @@
-import { useEffect, useRef, type ChangeEvent, type KeyboardEvent } from 'react';
+import { useRef, type ChangeEvent, type KeyboardEvent } from 'react';
+import { useMountEffect } from '@/hooks/useMountEffect';
 import { Search, X } from 'lucide-react';
 import { Button } from '@/components/ui/primitives/Button';
 import { cn } from '@/utils/cn';
@@ -37,7 +38,7 @@ export function SearchInput({
     inputRef.current?.focus();
   };
 
-  useEffect(() => {
+  useMountEffect(() => {
     const handleKeyboardShortcut = (event: globalThis.KeyboardEvent) => {
       if ((event.ctrlKey || event.metaKey) && event.key === 'f') {
         const activeElement = document.activeElement;
@@ -55,7 +56,7 @@ export function SearchInput({
 
     window.addEventListener('keydown', handleKeyboardShortcut);
     return () => window.removeEventListener('keydown', handleKeyboardShortcut);
-  }, []);
+  });
 
   return (
     <div role="search" className={cn('relative flex items-center', className)}>