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

gricha · 2026-02-08T19:16:45Z

Summary

Test PR with intentional React anti-patterns to verify warden review catches them

github-actions · 2026-02-08T19:18:34Z

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


⚠️ [JA7-PZU] Missing cleanup for global event listener (high confidence)

window resize event listener is added but never removed, causing memory leak and duplicate listeners on component remounts.

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:18:35Z

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


🟠 [MC2-X2M] Unnecessary effect for derived state (high confidence)

workspaceCount is derived from workspaces.length and should be calculated during render, not synced via useEffect which causes extra re-renders.

Suggested fix: Remove useEffect and 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:18:35Z

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


🟠 [3J4-9TC] Unnecessary effect - derive state during render instead (high confidence)

workspaceCount is derived from workspaces and should be calculated during render, not in an effect. This causes extra re-renders.

Suggested fix: Remove the effect and derive the value 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:18:35Z

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


🟠 [REH-E7U] Event listener cleanup missing (high confidence)

Resize listener added but never removed, causing 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:18:36Z

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


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

resize event listener is never removed, causing memory leak. Add cleanup function to useEffect.

Suggested fix: Return cleanup function from useEffect to remove the 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:18:36Z

@@ -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]);


🟠 [73B-VKA] Redundant state: workspaceCount derived from workspaces (high confidence)

workspaceCount duplicates data already available via workspaces?.length. Remove state and use derived value directly.

Suggested fix: Remove workspaceCount state and its useEffect. Use workspaces?.length directly where needed.

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

github-actions · 2026-02-08T19:18:36Z

@@ -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);
+  }, []);


🟠 [A5Z-42X] Redundant state: sidebarWidth measured from DOM (high confidence)

sidebarWidth state duplicates CSS-controlled width. Remove state and DOM measurement logic.

Suggested fix: Remove sidebarWidth state and both useEffect hooks that measure/update it (lines 54-59 and 105-111).

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

sentry · 2026-02-08T19:19:18Z

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


Bug: The list of workspaces uses the array index as the key prop, which is an anti-pattern that can cause rendering bugs and state corruption with dynamic lists.
_{Severity: MEDIUM}

Suggested Fix

Change the key prop for the mapped component from key={index} to a stable, unique identifier from the data, such as key={ws.name}.

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: The list of workspaces is rendered using the array `index` as the `key` prop. This is a React anti-pattern for dynamic lists where items can be added, removed, or reordered. Using an unstable key like `index` can lead to incorrect component state, rendering issues, and unpredictable behavior as React may incorrectly reconcile the list items. The `ws.name` property is available and should be used as a stable, unique key.

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

feat: add workspace count and sidebar improvements

aa96735

github-actions bot requested changes Feb 8, 2026

View reviewed changes

gricha closed this Feb 8, 2026

sentry bot reviewed Feb 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: warden react best practices review#157

test: warden react best practices review#157
gricha wants to merge 1 commit intomainfrom
test/react-violations

gricha commented Feb 8, 2026

Uh oh!

github-actions bot Feb 8, 2026

Uh oh!

github-actions bot Feb 8, 2026

Uh oh!

github-actions bot Feb 8, 2026

Uh oh!

github-actions bot Feb 8, 2026

Uh oh!

github-actions bot Feb 8, 2026

Uh oh!

github-actions bot Feb 8, 2026

Uh oh!

github-actions bot Feb 8, 2026

Uh oh!

sentry bot Feb 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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!

sentry bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant