From a19c369948a1c9b943aa2c08b6295a97e1db55de Mon Sep 17 00:00:00 2001 From: Stefan du Fresne Date: Tue, 20 Jan 2026 19:13:29 +0000 Subject: [PATCH 1/2] Refactor debug filter from useEffect to Redux middleware Move debug filter initialization and sync logic out of DebugManager's useEffect and into Redux middleware. This eliminates the anti-pattern of dispatching state updates in effects that should be computed at init time. - Initialize debug state from localStorage at store creation - Use debugMiddleware to sync state changes to localStorage/debugModule - Remove DebugManager component (no longer needed) Co-Authored-By: Claude Opus 4.5 --- src/client/App.tsx | 2 -- src/client/features/Debug/DebugManager.tsx | 29 ------------------ src/client/features/Debug/debugSlice.ts | 35 +++++++++++++++++++++- src/client/store.ts | 3 +- 4 files changed, 36 insertions(+), 33 deletions(-) delete mode 100644 src/client/features/Debug/DebugManager.tsx diff --git a/src/client/App.tsx b/src/client/App.tsx index f497a73..89bac9c 100644 --- a/src/client/App.tsx +++ b/src/client/App.tsx @@ -3,7 +3,6 @@ import './App.scss'; import { Typography } from '@mui/material'; import { lazy, Suspense } from 'react'; import { Route, Routes } from 'react-router-dom'; -import DebugManager from './features/Debug/DebugManager'; import Page from './features/Page/Page'; import SyncManager from './features/Sync/SyncManager'; import UpdateManager from './features/Update/UpdateManager'; @@ -24,7 +23,6 @@ function App() { return ( - {process.env.NODE_ENV !== 'development' && } diff --git a/src/client/features/Debug/DebugManager.tsx b/src/client/features/Debug/DebugManager.tsx deleted file mode 100644 index 32378ed..0000000 --- a/src/client/features/Debug/DebugManager.tsx +++ /dev/null @@ -1,29 +0,0 @@ -import debugModule from 'debug'; -import { useEffect } from 'react'; -import { useDispatch, useSelector } from '../../store'; -import { set as setDebug } from './debugSlice'; - -const DEBUG_KEY = 'debug-filter'; - -function DebugManager() { - const dispatch = useDispatch(); - const debugFilter = useSelector((state) => state.debug.value); - - useEffect(() => { - if (debugFilter === undefined) { - // uninitialized, look up from local store - dispatch(setDebug(localStorage.getItem(DEBUG_KEY))); - } else if (debugFilter === null) { - // no debugging set - debugModule.disable(); - localStorage.removeItem(DEBUG_KEY); - } else { - debugModule.enable(debugFilter); - localStorage.setItem(DEBUG_KEY, debugFilter); - } - }, [debugFilter, dispatch]); - - return null; -} - -export default DebugManager; diff --git a/src/client/features/Debug/debugSlice.ts b/src/client/features/Debug/debugSlice.ts index d102e73..297d4f6 100644 --- a/src/client/features/Debug/debugSlice.ts +++ b/src/client/features/Debug/debugSlice.ts @@ -1,8 +1,23 @@ +import type { Middleware } from '@reduxjs/toolkit'; import { createSlice } from '@reduxjs/toolkit'; +import debugModule from 'debug'; + +const DEBUG_KEY = 'debug-filter'; + +// Initialize from localStorage at module load time +function getInitialDebugValue(): string | null { + const stored = localStorage.getItem(DEBUG_KEY); + if (stored) { + debugModule.enable(stored); + } else { + debugModule.disable(); + } + return stored; +} export const debugSlice = createSlice({ name: 'debug', - initialState: { value: undefined as unknown as string }, + initialState: { value: getInitialDebugValue() }, reducers: { set: (state, action) => { state.value = action.payload; @@ -10,5 +25,23 @@ export const debugSlice = createSlice({ }, }); +// Middleware to sync debug state changes to localStorage and debugModule +export const debugMiddleware: Middleware = () => (next) => (action) => { + const result = next(action); + + if (debugSlice.actions.set.match(action)) { + const newValue = action.payload; + if (newValue === null) { + debugModule.disable(); + localStorage.removeItem(DEBUG_KEY); + } else if (newValue) { + debugModule.enable(newValue); + localStorage.setItem(DEBUG_KEY, newValue); + } + } + + return result; +}; + export const { set } = debugSlice.actions; export default debugSlice.reducer; diff --git a/src/client/store.ts b/src/client/store.ts index 249d5f0..07ec01e 100644 --- a/src/client/store.ts +++ b/src/client/store.ts @@ -1,7 +1,7 @@ import { configureStore } from '@reduxjs/toolkit'; import * as RR from 'react-redux'; -import debugSlice from './features/Debug/debugSlice'; +import debugSlice, { debugMiddleware } from './features/Debug/debugSlice'; import pageSlice from './features/Page/pageSlice'; import syncSlice from './features/Sync/syncSlice'; import updateSlice from './features/Update/updateSlice'; @@ -18,6 +18,7 @@ function createStore() { debug: debugSlice, update: updateSlice, }, + middleware: (getDefaultMiddleware) => getDefaultMiddleware().concat(debugMiddleware), // devTools: process.env.NODE_ENV !== 'production', }); } From 31628fc1797c485aa6d2e44a16dfb4fc288e268d Mon Sep 17 00:00:00 2001 From: Stefan du Fresne Date: Tue, 20 Jan 2026 19:25:59 +0000 Subject: [PATCH 2/2] Fix debug filter not clearing when text field is emptied Empty string "" is falsy but was not handled by the middleware - only null was explicitly checked. Now treat any falsy value as a clear. Co-Authored-By: Claude Opus 4.5 --- src/client/features/Debug/debugSlice.ts | 5 +- useEffectGone.md | 202 ++++++++++++++++++++++++ 2 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 useEffectGone.md diff --git a/src/client/features/Debug/debugSlice.ts b/src/client/features/Debug/debugSlice.ts index 297d4f6..bea5e89 100644 --- a/src/client/features/Debug/debugSlice.ts +++ b/src/client/features/Debug/debugSlice.ts @@ -31,10 +31,11 @@ export const debugMiddleware: Middleware = () => (next) => (action) => { if (debugSlice.actions.set.match(action)) { const newValue = action.payload; - if (newValue === null) { + if (!newValue) { + // null or empty string - clear debug debugModule.disable(); localStorage.removeItem(DEBUG_KEY); - } else if (newValue) { + } else { debugModule.enable(newValue); localStorage.setItem(DEBUG_KEY, newValue); } diff --git a/useEffectGone.md b/useEffectGone.md new file mode 100644 index 0000000..a3a7381 --- /dev/null +++ b/useEffectGone.md @@ -0,0 +1,202 @@ +# useEffect Elimination Plan + +Based on React's guidance from [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect), this document identifies useEffect instances that are incorrectly used and should be refactored. + +## Quick Reference: When to Avoid useEffect + +1. **Transforming data for rendering** → Calculate during render or use `useMemo` +2. **Handling user events** → Use event handlers +3. **Resetting state on prop change** → Use `key` prop +4. **Subscribing to external stores** → Use `useSyncExternalStore` +5. **Initializing the app** → Use module-level code + +--- + +## LOW RISK - Safe to Refactor + +These useEffects can be removed with minimal impact and clear alternatives. + +### 1. Page Context Effects (5 instances) + +**Pattern**: Dispatching `setContext()` on mount to set page title/navigation state. + +| File | Lines | Current Code | +|------|-------|--------------| +| [About.tsx](src/client/pages/About.tsx#L68-L76) | 68-76 | `useEffect(() => dispatch(setContext({title: 'About', ...})), [dispatch])` | +| [History.tsx](src/client/pages/History.tsx#L20-L27) | 20-27 | `useEffect(() => dispatch(setContext({title: 'History', ...})), [dispatch])` | +| [Home.tsx](src/client/pages/Home.tsx#L28-L35) | 28-35 | `useEffect(() => dispatch(setContext({title: 'Repeatable Checklists', ...})), [dispatch])` | +| [Template.tsx](src/client/pages/Template.tsx#L62-L70) | 62-70 | `useEffect(() => dispatch(setContext({title: template?.title, ...})))` - **No deps array!** | +| [Repeatable.tsx](src/client/pages/Repeatable.tsx#L37-L100) | 37-100 | Nested inside larger effect (context set at line ~80) | + +**Why Remove**: These are setting state during render based on props/route. This is a classic anti-pattern - dispatching to set derived state that could be computed. + +**Recommended Approach**: +1. Create a custom hook `usePageContext` that calls `dispatch(setContext(...))` during render +2. Or use a layout component pattern where context is set declaratively +3. For Template.tsx specifically: the missing dependency array means it runs on EVERY render - this is a bug + +**Example Refactor**: +```typescript +// Option 1: Custom hook that sets context during render +function usePageContext(context: PageContext) { + const dispatch = useAppDispatch(); + const contextRef = useRef(context); + if (!shallowEqual(contextRef.current, context)) { + contextRef.current = context; + } + useMemo(() => { + dispatch(setContext(contextRef.current)); + }, [dispatch, contextRef.current]); +} + +// Usage +function About() { + usePageContext({ title: 'About', back: true, under: 'about' }); + // ... +} +``` + +**Risk Level**: LOW +- No external systems involved +- No async operations +- Easy to test +- Clear migration path + +--- + +### 2. Debug Filter Management + +**File**: [DebugManager.tsx](src/client/features/Debug/DebugManager.tsx#L12-L24) (lines 12-24) + +**Current Code**: +```typescript +useEffect(() => { + if (debugFilter === undefined) { + dispatch(setDebug(localStorage.getItem(DEBUG_KEY))); + } else if (debugFilter === null) { + debugModule.disable(); + localStorage.removeItem(DEBUG_KEY); + } else { + debugModule.enable(debugFilter); + localStorage.setItem(DEBUG_KEY, debugFilter); + } +}, [debugFilter, dispatch]); +``` + +**Why Remove**: This is initializing state from localStorage and then syncing state changes back. The initialization should happen once at app startup, and the sync should be handled by Redux middleware - not a render effect. + +**Recommended Approach**: +1. Initialize debug filter in Redux initial state (read from localStorage at store creation) +2. Use Redux middleware to sync localStorage on state changes +3. Call `debugModule.enable/disable` from the middleware + +**Risk Level**: LOW +- Debug-only feature +- No user-facing impact if it breaks +- Simple state management pattern + +--- + +## MEDIUM RISK - Refactor with Caution + +These require more careful consideration and testing. + +### 3. Update Manager - Install Update + +**File**: [UpdateManager.tsx](src/client/features/Update/UpdateManager.tsx#L27-L33) (lines 27-33) + +**Current Code**: +```typescript +useEffect(() => { + if (registration && waitingToInstall && userReadyToUpdate) { + registration.waiting?.postMessage({ type: 'SKIP_WAITING' }); + } +}, [registration, userReadyToUpdate, waitingToInstall]); +``` + +**Why Remove**: This is reacting to state changes to trigger an action. The `userReadyToUpdate` flag is set by a user clicking a button - the `postMessage` call should happen directly in that click handler, not as a cascading effect. + +**Recommended Approach**: +- Pass `registration` to the component with the update button +- Call `registration.waiting?.postMessage({ type: 'SKIP_WAITING' })` directly in the click handler +- Remove `userReadyToUpdate` state entirely + +**Risk Level**: MEDIUM +- Critical functionality (PWA updates) +- But relatively simple logic +- Straightforward event-driven refactor + +--- + +### 4. Stale Queue Processing + +**File**: [SyncManager.tsx](src/client/features/Sync/SyncManager.tsx#L211-L228) (lines 211-228) + +**Current Code**: +```typescript +useEffect(() => { + const docs = Object.values(stale); + if (docs.length && socket && socket.connected && state === State.connected) { + socket.emit('docUpdate', docs); + } + dispatch(cleanStale(docs)); +}, [dispatch, socket, stale, state]); +``` + +**Why Remove**: This processes a queue whenever `stale` changes. The stale queue is populated by `userPut` operations - the socket emit should happen as part of that action flow, not as a reactive effect. + +**Recommended Approach**: +- Move to Redux middleware that listens for actions that add to stale queue +- Emit socket events directly from the middleware when docs are added +- Or use a Redux thunk that handles both the local update and socket emit + +**Risk Level**: MEDIUM +- Core sync functionality +- Logic is straightforward +- Need to ensure socket availability in middleware context + +--- + +### 5. Socket Ready Signal + +**File**: [SyncManager.tsx](src/client/features/Sync/SyncManager.tsx#L230-L237) (lines 230-237) + +**Current Code**: +```typescript +useEffect(() => { + if (socket?.connected && state === State.completed) { + socket.emit('ready'); + dispatch(socketConnected()); + } +}, [dispatch, socket, state]); +``` + +**Why Remove**: This is reacting to sync completion to emit a ready signal. The sync completion happens in another useEffect - the ready signal should be emitted directly at the end of that sync logic, not as a cascading effect. + +**Recommended Approach**: +- At the end of the full sync handler (after `dispatch(completeSync())`), emit the ready signal and dispatch `socketConnected()` directly +- This makes the flow explicit rather than reactive + +**Risk Level**: MEDIUM +- Part of sync flow +- Simple conditional logic +- Timing might be tricky - need to ensure socket is available + +--- + +## Summary + +| Risk | Count | Description | +|------|-------|-------------| +| LOW | 6 | 5 page context effects + debug filter management | +| MEDIUM | 3 | Update install trigger, stale queue processing, socket ready signal | + +**Total**: 9 useEffects to refactor + +## Recommended Priority + +1. **First**: Fix the Template.tsx missing dependency array (bug!) +2. **Second**: Refactor page context effects (LOW risk, 5 instances, same pattern) +3. **Third**: Move debug filter to Redux middleware (LOW risk) +4. **Fourth**: Refactor UpdateManager install trigger to event handler (MEDIUM risk) +5. **Fifth**: Refactor SyncManager stale queue and ready signal (MEDIUM risk, related changes)