Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 10 additions & 7 deletions src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { TaskBoard, TaskProvider } from './features/tasks';
import { AuthProvider, useAuth } from './features/auth';
import { ProjectProvider } from './features/projects';
import { ToastProvider } from './shared/components/Toast';
import { ErrorBoundary } from './shared/components/ErrorBoundary';
import { GlobalSearch, SearchTrigger } from './features/search';
import { Spinner } from './shared/components/Loading';
import { Avatar } from './shared/components/Avatar';
Expand Down Expand Up @@ -485,13 +486,15 @@ function AppLayout() {

function App() {
return (
<ToastProvider>
<AuthProvider>
<ProjectProvider>
<AppLayout />
</ProjectProvider>
</AuthProvider>
</ToastProvider>
<ErrorBoundary>
<ToastProvider>
<AuthProvider>
<ProjectProvider>
<AppLayout />
</ProjectProvider>
</AuthProvider>
</ToastProvider>
</ErrorBoundary>
Comment on lines +489 to +497

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

🐛 Bug: ErrorBoundary placement outside ToastProvider prevents error notifications

The ErrorBoundary wraps the ToastProvider, which means if an error occurs, the toast system won't be available to display error messages. This breaks the user feedback mechanism during errors.

-    <ErrorBoundary>
-      <ToastProvider>
+    <ToastProvider>
+      <ErrorBoundary>
        <AuthProvider>
          <ProjectProvider>
            <AppLayout />
          </ProjectProvider>
        </AuthProvider>
-      </ToastProvider>
-    </ErrorBoundary>
+      </ErrorBoundary>
+    </ToastProvider>

Alternative consideration: If ErrorBoundary needs to catch ToastProvider errors, consider having two ErrorBoundaries - one outside for ToastProvider errors (with basic fallback), and one inside for application errors (with toast notifications).

);
}

Expand Down
25 changes: 19 additions & 6 deletions src/core/api/http-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,18 @@ class HttpClient {
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), timeout);

const response = await fetch(fullUrl, {
...fetchConfig,
headers,
body,
signal: controller.signal,
});
let response: Response;
try {
response = await fetch(fullUrl, {
...fetchConfig,
headers,
body,
signal: controller.signal,
});
} catch (fetchError) {
clearTimeout(timeoutId);
throw fetchError;
}

clearTimeout(timeoutId);

Expand Down Expand Up @@ -317,6 +323,13 @@ class HttpClient {
};
}

if (error.message.includes('Failed to fetch') || error.message.includes('NetworkError')) {
return {
code: ERROR_CODES.NETWORK_ERROR,
message: 'Network connection failed. Please check your internet connection.',
};
}
Comment on lines +326 to +331

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

🐛 Bug: Fragile error detection relies on string matching which is locale-dependent and browser-specific

The error message matching approach is unreliable because:

  1. Error messages vary across browsers (Chrome: "Failed to fetch", Firefox: "NetworkError when attempting to fetch resource")
  2. Messages may be localized in non-English browsers
  3. The check happens AFTER the generic network error check, making it unreachable code
-      if (error.message.includes('Failed to fetch') || error.message.includes('NetworkError')) {
+      // Check for network errors by error type/name instead of message
+      if (error.name === 'TypeError' || error instanceof TypeError) {
         return {
           code: ERROR_CODES.NETWORK_ERROR,
           message: 'Network connection failed. Please check your internet connection.',
         };
       }

       return {
         code: ERROR_CODES.NETWORK_ERROR,
         message: error.message || 'Network error occurred',

Note: This code block appears to be unreachable since it comes after a generic return statement for network errors. Consider restructuring the error handling logic to check for specific network error types first, then fall back to the generic handler.


return {
code: ERROR_CODES.NETWORK_ERROR,
message: error.message || 'Network error occurred',
Expand Down
1 change: 1 addition & 0 deletions src/core/hooks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,4 @@ export { useInterval, useTimeout, useCountdown, useTimer, usePolling } from './u
export { useToggle } from './useToggle';
export { useDisclosure, useDisclosures, useModal, useConfirmDialog } from './useDisclosure';
export type { UseDisclosureReturn, UseDisclosureOptions } from './useDisclosure';
export { useThrottle, useIdleCallback, useIntersectionObserver } from './usePerformance';
85 changes: 85 additions & 0 deletions src/core/hooks/usePerformance.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import { useEffect, useRef, useCallback } from 'react';

export function useThrottle<T extends (...args: unknown[]) => unknown>(
callback: T,
delay: number
): T {
const lastRun = useRef<number>(0);
const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);

const throttledCallback = useCallback(
((...args: Parameters<T>) => {
const now = Date.now();
const timeSinceLastRun = now - lastRun.current;

if (timeSinceLastRun >= delay) {
lastRun.current = now;
return callback(...args);
}

if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}

timeoutRef.current = setTimeout(() => {
lastRun.current = Date.now();
callback(...args);
}, delay - timeSinceLastRun);
}) as T,
[callback, delay]
);

useEffect(() => {
return () => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}
};
}, []);

return throttledCallback;
}
Comment on lines +3 to +41

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

severity: medium
🐛 Bug: useThrottle implementation has stale closure issue and incorrect throttle behavior

The throttledCallback doesn't include callback in its dependencies properly, and the throttle logic is flawed. When a throttled call is scheduled, it uses the delay from the first call rather than ensuring the callback runs at the end of the throttle window. Additionally, the callback reference can become stale.

-export function useThrottle<T extends (...args: unknown[]) => unknown>(
-  callback: T,
-  delay: number
-): T {
-  const lastRun = useRef<number>(0);
-  const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
-
-  const throttledCallback = useCallback(
-    ((...args: Parameters<T>) => {
-      const now = Date.now();
-      const timeSinceLastRun = now - lastRun.current;
-
-      if (timeSinceLastRun >= delay) {
-        lastRun.current = now;
-        return callback(...args);
-      }
-
-      if (timeoutRef.current) {
-        clearTimeout(timeoutRef.current);
-      }
-
-      timeoutRef.current = setTimeout(() => {
-        lastRun.current = Date.now();
-        callback(...args);
-      }, delay - timeSinceLastRun);
-    }) as T,
-    [callback, delay]
-  );
+export function useThrottle<T extends (...args: unknown[]) => unknown>(
+  callback: T,
+  delay: number
+): T {
+  const lastRun = useRef<number>(0);
+  const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+  const callbackRef = useRef(callback);
+
+  useEffect(() => {
+    callbackRef.current = callback;
+  }, [callback]);
+
+  const throttledCallback = useCallback(
+    ((...args: Parameters<T>) => {
+      const now = Date.now();
+      const timeSinceLastRun = now - lastRun.current;
+
+      if (timeSinceLastRun >= delay) {
+        lastRun.current = now;
+        return callbackRef.current(...args);
+      }
+
+      if (timeoutRef.current) {
+        clearTimeout(timeoutRef.current);
+      }
+
+      timeoutRef.current = setTimeout(() => {
+        lastRun.current = Date.now();
+        callbackRef.current(...args);
+      }, delay - timeSinceLastRun);
+    }) as T,
+    [delay]
+  );


export function useIdleCallback(callback: () => void, deps: unknown[] = []) {
const callbackRef = useRef(callback);

useEffect(() => {
callbackRef.current = callback;
}, [callback]);

useEffect(() => {
if (typeof requestIdleCallback !== 'undefined') {
const id = requestIdleCallback(() => {
callbackRef.current();
});
return () => cancelIdleCallback(id);
} else {
const timeoutId = setTimeout(() => {
callbackRef.current();
}, 1);
return () => clearTimeout(timeoutId);
}
}, deps);
}
Comment on lines +43 to +63

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Pattern Detected: Bug: Unstable Dependencies Causing Hook Re-execution (3 occurrences)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Severity: HIGH

This issue appears in 3 locations across 2 files. Each location requires specific attention based on its context.

📍 Affected Locations:
src/core/hooks/usePerformance.ts:63
src/core/hooks/usePerformance.ts:84
src/features/tasks/components/TaskBoard.tsx:100

💡 General Guidance:

use the effect to re-run on every render if deps is a new array instance (which it will be if passed inline). The effect should use a stable reference or spread the deps.

📋 Specific Suggestions for Each File (Click to expand)

src/core/hooks/usePerformance.ts:63

🟡 Medium

Fix: use the effect to re-run on every render if deps is a new array instance (which it will be if passed inline). The effect should use a stable reference or spread the deps.


src/core/hooks/usePerformance.ts:84

🟡 Medium

Fix: try]) => {

  •  callback(entry.isIntersecting);
    
  • }, options);
  •  callbackRef.current(entry.isIntersecting);
    
  • }, optionsRef.current);

    observer.observe(element);

    return () => {
    observer.disconnect();
    };

  • }, [elementRef, callback, options]);
  • }, [elementRef]);
    }


---

#### `src/features/tasks/components/TaskBoard.tsx:100`

🟡 **Medium**

**Fix:** try {
+                clearError();
+              } catch (err) {
+                console.error('Failed to clear error:', err);
+              }
+            }}
             aria-label="Dismiss error"
           >


</details>

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
💡 **Tip:** Fixing this pattern across all locations will improve code quality consistently.


<details>
<summary>🤖 <b>AI Fix Prompt</b> - Copy this to your AI assistant</summary>

<blockquote>

Fix the unstable dependency arrays causing unnecessary hook re-executions in src/core/hooks/usePerformance.ts at lines 43-63, 63-84 and src/features/tasks/components/TaskBoard.tsx at line 100

The problem is that dependency arrays are being passed as inline array literals to useEffect and useCallback hooks. Every time the component renders, a new array instance is created even if the values inside are the same. This causes React to think the dependencies have changed, triggering unnecessary effect re-runs and callback recreations. This defeats the purpose of memoization and can lead to performance issues and infinite render loops.

In usePerformance.ts there are two specific issues:

First issue around line 63 in the useThrottle or useIdleCallback hook - the deps array is likely being passed inline like [deps] or the callback itself is not stable. This needs to be fixed by either using a ref for the callback or ensuring the deps array has a stable reference.

Second issue around line 84 in the useIntersectionObserver hook - both the callback and options object are being passed directly in the dependency array. Since functions and objects are recreated on each render, this causes the effect to run every time. The fix is to use refs to store the callback and options, then access them via callbackRef.current and optionsRef.current inside the effect. The dependency array should only include elementRef.

Third issue in TaskBoard.tsx around line 100 - similar pattern where an inline function or array is causing instability.

The correct pattern for these hooks is:

For callbacks that need to be stable - wrap them in useCallback with proper dependencies or store them in a ref using useRef and update the ref in a separate useEffect

For objects like options - either memoize them with useMemo outside the hook or store them in a ref

For dependency arrays - never pass inline arrays, always define them as a const variable with proper memoization or use the spread operator on a stable reference

Look at how other custom hooks in the codebase handle this pattern. Check if there are examples of useCallback, useMemo, or useRef being used to stabilize dependencies in other hook files.

Files that need changes:
src/core/hooks/usePerformance.ts - fix both the useThrottle/useIdleCallback and useIntersectionObserver implementations
src/features/tasks/components/TaskBoard.tsx - fix the dependency issue in the error handling code

Requirements:
Ensure all callbacks used in effects are either wrapped in useCallback or stored in refs
Ensure all object dependencies like options are memoized or stored in refs
Remove function and object references from dependency arrays and replace with refs
Keep the dependency arrays minimal - only include primitive values or stable refs
Test that the hooks no longer re-run on every render by adding console logs
Verify that the actual functionality still works correctly after the changes
Make sure the fixes follow React hooks best practices and ESLint rules for exhaustive-deps

</blockquote>

</details>


export function useIntersectionObserver(
elementRef: React.RefObject<Element>,
callback: (isIntersecting: boolean) => void,
options?: IntersectionObserverInit
) {
useEffect(() => {
const element = elementRef.current;
if (!element) return;

const observer = new IntersectionObserver(([entry]) => {
callback(entry.isIntersecting);
}, options);

observer.observe(element);

return () => {
observer.disconnect();
};
}, [elementRef, callback, options]);
}

Loading