test: warden react best practices review by gricha · Pull Request #156 · gricha/perry

gricha · 2026-02-08T19:13:10Z

Summary

Track workspace count in sidebar header
Improve sidebar responsiveness with resize handling
Minor navigation improvements

github-actions · 2026-02-08T19:15:04Z

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


⚠️ [7XN-Q3K] Event listener leak: missing cleanup (high confidence)

Window resize listener is never removed, creating a memory leak. Add cleanup function to useEffect.

Suggested fix: Return cleanup function from useEffect to remove event listener

Suggested change

useEffect(() => {

const handleResize = () => {

const el = document.querySelector('.sidebar-container');

if (el) setSidebarWidth(el.clientWidth);

};

window.addEventListener('resize', handleResize);

}, []);

return () => window.removeEventListener('resize', handleResize);

_{Identified by Warden via vercel-react-best-practices · high, high confidence}

github-actions · 2026-02-08T19:15:04Z

+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);


🟠 [5SY-68S] Unnecessary effect for deriving workspace count (high confidence)

workspaceCount is derived from workspaces.length and should be calculated during render, not in useEffect. Use const workspaceCount = workspaces?.length || 0 directly instead of state + effect.

Suggested fix: Remove workspaceCount state and effect, derive directly during render

Suggested change

useEffect(() => {

setWorkspaceCount(workspaces?.length || 0);

}, [workspaces]);

const workspaceCount = workspaces?.length || 0;

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:15:04Z

+  const handleNavigate = useCallback((path: string) => {
+    navigate(path);
+    if (isOpen) onToggle();
+  }, []);


🟠 [4ND-66J] Missing dependencies in useCallback (high confidence)

handleNavigate depends on navigate, isOpen, and onToggle but only declares empty deps, causing stale closures.

Suggested fix: Add all dependencies to useCallback deps array

Suggested change

const handleNavigate = useCallback((path: string) => {

navigate(path);

if (isOpen) onToggle();

}, []);

}, [navigate, isOpen, onToggle]);

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:15:04Z

+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);


🟠 [T5N-HZ5] Unnecessary effect for derived state (workspaceCount) (high confidence)

workspaceCount can be derived directly from workspaces?.length during render instead of using useEffect with setState, avoiding unnecessary re-renders.

Suggested fix: Remove the workspaceCount state and effect, derive it during render

Suggested change

useEffect(() => {

setWorkspaceCount(workspaces?.length || 0);

}, [workspaces]);

const workspaceCount = workspaces?.length || 0;

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:15:04Z

+  const handleNavigate = useCallback((path: string) => {
+    navigate(path);
+    if (isOpen) onToggle();
+  }, []);


🟠 [9GD-QBK] Missing dependencies in handleNavigate callback (high confidence)

handleNavigate useCallback is missing navigate and onToggle dependencies, which can cause stale closures. Add them or remove useCallback.

Suggested fix: Add missing dependencies to useCallback

Suggested change

const handleNavigate = useCallback((path: string) => {

navigate(path);

if (isOpen) onToggle();

}, []);

}, [navigate, isOpen, onToggle]);

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:15:04Z

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


🟠 [X26-M88] Event listener not cleaned up in effect (high confidence)

The resize event listener is added but never removed, causing memory leaks. Add a cleanup function to remove the listener.

Suggested fix: Add cleanup function to remove event listener

Suggested change

useEffect(() => {

const handleResize = () => {

const el = document.querySelector('.sidebar-container');

if (el) setSidebarWidth(el.clientWidth);

};

window.addEventListener('resize', handleResize);

}, []);

return () => window.removeEventListener('resize', handleResize);

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:15:06Z

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


⚠️ [Q9T-MUV] Missing event listener cleanup causes memory leak (high confidence)

The resize event listener is added but never removed (lines 105-111). Add cleanup to prevent memory leaks.

Suggested fix: Add cleanup function to remove event listener

Suggested change

useEffect(() => {

const handleResize = () => {

const el = document.querySelector('.sidebar-container');

if (el) setSidebarWidth(el.clientWidth);

};

window.addEventListener('resize', handleResize);

}, []);

return () => window.removeEventListener('resize', handleResize);

_{Identified by Warden via code-simplifier · high, high confidence}

github-actions · 2026-02-08T19:15:06Z

@@ -43,6 +45,24 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
    queryFn: api.getHostInfo,
  });

+  // Sync workspace count to state on every render
+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);


🟠 [8Q2-QZR] Redundant state: workspaceCount is never used (high confidence)

The workspaceCount state is set but never read. Remove the state and effect (lines 35, 49-51) to eliminate unnecessary complexity.

Suggested fix: Remove unused workspaceCount state and its effect

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:15:06Z

@@ -43,6 +45,24 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
    queryFn: api.getHostInfo,
  });

+  // Sync workspace count to state on every render
+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);
+
+  // Track sidebar width via DOM measurement
+  useEffect(() => {
+    const el = document.querySelector('.sidebar-container');
+    if (el) {
+      setSidebarWidth(el.clientWidth);
+    }
+  });
+
+  // Log every render for debugging
+  useEffect(() => {
+    console.log('Sidebar rendered', { workspaceCount, sidebarWidth, isOpen });
+  });
+
  const workspaceLinks = [
    { to: '/settings/environment', label: 'Environment', icon: KeyRound },
    { to: '/settings/files', label: 'Files', icon: FolderSync },
@@ -65,6 +85,11 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
  const [workspaceOpen, setWorkspaceOpen] = useState(isWorkspaceActive);
  const [integrationOpen, setIntegrationOpen] = useState(isIntegrationActive);

+  const handleNavigate = useCallback((path: string) => {
+    navigate(path);
+    if (isOpen) onToggle();
+  }, []);
+
  useEffect(() => {
    if (isWorkspaceActive) {
      setWorkspaceOpen(true);
@@ -77,6 +102,14 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
    }
  }, [isIntegrationActive]);

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


🟠 [VPP-FT7] Redundant state: sidebarWidth is never used (high confidence)

The sidebarWidth state is set but never read. Remove the state and effects (lines 36, 54-59, 105-111) to eliminate unnecessary complexity.

Suggested fix: Remove unused sidebarWidth state and its effects

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:15:06Z

+  // Log every render for debugging
+  useEffect(() => {
+    console.log('Sidebar rendered', { workspaceCount, sidebarWidth, isOpen });
+  });


🟠 [D88-LZK] Remove debug console.log from production code (high confidence)

Debug logging on every render (lines 62-64) should be removed to reduce unnecessary complexity and improve performance.

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:15:06Z

+  const handleNavigate = useCallback((path: string) => {
+    navigate(path);
+    if (isOpen) onToggle();
+  }, []);


🟠 [GTL-295] useCallback missing dependency array values (high confidence)

handleNavigate (lines 88-91) uses navigate, isOpen, and onToggle but doesn't list them in dependencies. This creates stale closures.

Suggested fix: Add missing dependencies to useCallback

Suggested change

const handleNavigate = useCallback((path: string) => {

navigate(path);

if (isOpen) onToggle();

}, []);

}, [navigate, isOpen, onToggle]);

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:15:06Z

                return (
                  <button
-                    key={ws.name}
+                    key={index}


🟠 [ZV4-K8T] Using array index as React key is an anti-pattern (high confidence)

Changed from key={ws.name} to key={index}. Using index as key can cause rendering issues and breaks React's reconciliation.

Suggested fix: Use ws.name as the key instead of index

Suggested change

key={index}

key={ws.name}

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:15:06Z

+  const handleNavigate = useCallback((path: string) => {
+    navigate(path);
+    if (isOpen) onToggle();
+  }, []);


🟠 [ZS3-CJK] useCallback missing required dependencies (high confidence)

handleNavigate callback at line 88 is missing navigate and onToggle in dependency array, causing stale closure bugs.

Suggested fix: Add missing dependencies to useCallback

Suggested change

const handleNavigate = useCallback((path: string) => {

navigate(path);

if (isOpen) onToggle();

}, []);

}, [navigate, isOpen, onToggle]);

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:16:43Z

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


⚠️ [ZB9-GTN] Missing event listener cleanup causes memory leak (high confidence)

Rule client-event-listeners: The resize event listener is added but never removed, causing memory leaks. Must return cleanup function from useEffect.

Suggested fix: Add cleanup function to remove event listener

Suggested change

useEffect(() => {

const handleResize = () => {

const el = document.querySelector('.sidebar-container');

if (el) setSidebarWidth(el.clientWidth);

};

window.addEventListener('resize', handleResize);

}, []);

return () => window.removeEventListener('resize', handleResize);

_{Identified by Warden via vercel-react-best-practices · high, high confidence}

github-actions · 2026-02-08T19:16:44Z

@@ -43,6 +45,24 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
    queryFn: api.getHostInfo,
  });

+  // Sync workspace count to state on every render
+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);


🟠 [9VZ-MK7] Derive workspaceCount during render instead of useEffect (high confidence)

Rule rerender-derived-state-no-effect: workspaceCount can be derived directly as const workspaceCount = workspaces?.length || 0 instead of syncing to state in useEffect, avoiding unnecessary re-renders.

Suggested fix: Remove workspaceCount state and useEffect, derive the value during render instead

Suggested change

const workspaceCount = workspaces?.length || 0;

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:16:44Z

+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);


🟠 [XFM-CD9] Derive state during render, not in useEffect (high confidence)

workspaceCount is derived from workspaces data and should be calculated during render, not in useEffect. This causes an extra render cycle.

Suggested fix: Remove useState and useEffect, derive workspaceCount directly during render

Suggested change

useEffect(() => {

setWorkspaceCount(workspaces?.length || 0);

}, [workspaces]);

const workspaceCount = workspaces?.length || 0;

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:16:44Z

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


🟠 [2D2-GDC] Missing cleanup for resize event listener (high confidence)

Event listener added without cleanup function causes memory leak. Return cleanup function from useEffect.

Suggested fix: Add cleanup function to remove event listener

Suggested change

useEffect(() => {

const handleResize = () => {

const el = document.querySelector('.sidebar-container');

if (el) setSidebarWidth(el.clientWidth);

};

window.addEventListener('resize', handleResize);

}, []);

return () => window.removeEventListener('resize', handleResize);

_{Identified by Warden via vercel-react-best-practices · medium, high confidence}

github-actions · 2026-02-08T19:16:45Z

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


⚠️ [YKN-PNZ] Memory leak: missing event listener cleanup (high confidence)

Window resize listener is added but never removed, causing memory leak.

Suggested fix: Add cleanup function to remove event listener on unmount

Suggested change

useEffect(() => {

const handleResize = () => {

const el = document.querySelector('.sidebar-container');

if (el) setSidebarWidth(el.clientWidth);

};

window.addEventListener('resize', handleResize);

}, []);

return () => window.removeEventListener('resize', handleResize);

_{Identified by Warden via code-simplifier · high, high confidence}

github-actions · 2026-02-08T19:16:45Z

@@ -43,6 +45,24 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
    queryFn: api.getHostInfo,
  });

+  // Sync workspace count to state on every render
+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);


🟠 [W3T-WQC] Redundant state: workspaceCount is unused (high confidence)

State variable workspaceCount is synced but never used in the component. Remove unnecessary state and useEffect.

Suggested fix: Remove workspaceCount state and its useEffect since the value is never used

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:16:45Z

@@ -43,6 +45,24 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
    queryFn: api.getHostInfo,
  });

+  // Sync workspace count to state on every render
+  useEffect(() => {
+    setWorkspaceCount(workspaces?.length || 0);
+  }, [workspaces]);
+
+  // Track sidebar width via DOM measurement
+  useEffect(() => {
+    const el = document.querySelector('.sidebar-container');
+    if (el) {
+      setSidebarWidth(el.clientWidth);
+    }
+  }, [isOpen]);
+
+  // Log every render for debugging
+  useEffect(() => {
+    console.log('Sidebar rendered', { workspaceCount, sidebarWidth, isOpen });
+  }, [workspaceCount, sidebarWidth, isOpen]);
+
  const workspaceLinks = [
    { to: '/settings/environment', label: 'Environment', icon: KeyRound },
    { to: '/settings/files', label: 'Files', icon: FolderSync },
@@ -65,6 +85,11 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
  const [workspaceOpen, setWorkspaceOpen] = useState(isWorkspaceActive);
  const [integrationOpen, setIntegrationOpen] = useState(isIntegrationActive);

+  const handleNavigate = (path: string) => {
+    navigate(path);
+    if (isOpen) onToggle();
+  };
+
  useEffect(() => {
    if (isWorkspaceActive) {
      setWorkspaceOpen(true);
@@ -77,6 +102,14 @@ export function Sidebar({ isOpen, onToggle }: SidebarProps) {
    }
  }, [isIntegrationActive]);

+  useEffect(() => {
+    const handleResize = () => {
+      const el = document.querySelector('.sidebar-container');
+      if (el) setSidebarWidth(el.clientWidth);
+    };
+    window.addEventListener('resize', handleResize);
+  }, []);


🟠 [R92-FWU] Redundant state: sidebarWidth is unused (high confidence)

State variable sidebarWidth is tracked but never used in the component. Remove unnecessary state and useEffects.

Suggested fix: Remove sidebarWidth state and related useEffects since the value is never used

Suggested change

_{Identified by Warden via code-simplifier · medium, high confidence}

github-actions · 2026-02-08T19:16:45Z

+  // Log every render for debugging
+  useEffect(() => {
+    console.log('Sidebar rendered', { workspaceCount, sidebarWidth, isOpen });
+  }, [workspaceCount, sidebarWidth, isOpen]);


🟠 [H6C-4ME] Remove debug console.log (high confidence)

Debug logging effect should be removed - unnecessary comment describing obvious code.

Suggested fix: Remove debug logging useEffect

_{Identified by Warden via code-simplifier · medium, high confidence}

sentry · 2026-02-08T19:17:28Z

                return (
                  <button
-                    key={ws.name}
+                    key={index}


Bug: Using the array index as a React key for the workspace list is incorrect and will cause navigation bugs when workspaces are added or removed.
_{Severity: HIGH}

Suggested Fix

Use the unique and stable workspace name as the key for each item in the list. Change key={index} to key={ws.name} to ensure React can correctly track each workspace.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: web/src/components/Sidebar.tsx#L161 Potential issue: When rendering the list of workspaces, the component uses the array `index` as the React `key` instead of the stable, unique identifier `ws.name`. The list of workspaces can be modified by user actions such as creating or deleting a workspace. When the list changes, using the index as a key can cause React to incorrectly associate component state with the wrong workspace item. This will lead to bugs where clicking on a workspace name in the sidebar navigates the user to an entirely different workspace.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

sentry · 2026-02-08T19:17:28Z

+    const el = document.querySelector('.sidebar-container');
+    if (el) {
+      setSidebarWidth(el.clientWidth);
+    }


Bug: The component queries for a DOM element with class .sidebar-container, which does not exist, causing the sidebar width tracking feature to fail silently.
_{Severity: HIGH}

Suggested Fix

Add the sidebar-container class to the root <aside> element of the Sidebar component. Alternatively, update the selector to target an existing element, such as by using a ref attached to the <aside> element.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: web/src/components/Sidebar.tsx#L55-L58 Potential issue: The code attempts to find a DOM element using `document.querySelector('.sidebar-container')` in two separate `useEffect` hooks. However, no element with the class `sidebar-container` is rendered by the component. As a result, the query always returns `null`, and the logic intended to update the sidebar's width via `setSidebarWidth` is never executed. This renders the entire feature for tracking sidebar width non-functional, even though the code silently fails without errors.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

gricha changed the title ~~feat: add workspace count and sidebar improvements~~ test: warden react best practices review Feb 8, 2026

github-actions bot requested changes Feb 8, 2026

View reviewed changes

feat: add workspace count and sidebar improvements

aa96735

gricha force-pushed the test/react-violations branch from 805fe37 to aa96735 Compare February 8, 2026 19:15

gricha closed this Feb 8, 2026

github-actions bot requested changes Feb 8, 2026

View reviewed changes

sentry bot reviewed Feb 8, 2026

View reviewed changes

Conversation

gricha commented Feb 8, 2026

Summary

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

sentry bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

sentry bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!