Skip to content

test: warden react best practices review #156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gricha wants to merge 1 commit into from
Closed

test: warden react best practices review #156

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 37 additions & 6 deletions web/src/components/Sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,37 @@
export function Sidebar({ isOpen, onToggle }: SidebarProps) {
const location = useLocation();
const navigate = useNavigate();
const [workspaceCount, setWorkspaceCount] = useState(0);
const [sidebarWidth, setSidebarWidth] = useState(240);

const { data: workspaces } = useQuery({
queryKey: ['workspaces'],
queryFn: api.listWorkspaces,
});

const { data: hostInfo } = useQuery({
queryKey: ['hostInfo'],
queryFn: api.getHostInfo,
});

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

Check warning on line 51 in web/src/components/Sidebar.tsx

View workflow job for this annotation

GitHub Actions / warden: vercel-react-best-practices

Unnecessary effect for derived state 

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

Check warning on line 51 in web/src/components/Sidebar.tsx

View workflow job for this annotation

GitHub Actions / warden: vercel-react-best-practices

Unnecessary effect - derive state during render instead 

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

Check warning on line 51 in web/src/components/Sidebar.tsx

View workflow job for this annotation

GitHub Actions / warden: code-simplifier

Redundant state: workspaceCount derived from workspaces 

workspaceCount duplicates data already available via workspaces?.length. Remove state and use derived value directly.
Comment on lines +49 to +51
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines +49 to +51
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines 35 to +51
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines 35 to +51
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines +49 to +51
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines 35 to +51
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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


// Track sidebar width via DOM measurement
useEffect(() => {
const el = document.querySelector('.sidebar-container');
if (el) {
setSidebarWidth(el.clientWidth);
}
Comment on lines +55 to +58
Copy link
Copy Markdown

@sentry sentry bot Feb 8, 2026

Choose a reason for hiding this comment

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

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.

}, [isOpen]);

// Log every render for debugging
useEffect(() => {
console.log('Sidebar rendered', { workspaceCount, sidebarWidth, isOpen });
}, [workspaceCount, sidebarWidth, isOpen]);
Comment on lines +61 to +64
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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


const workspaceLinks = [
{ to: '/settings/environment', label: 'Environment', icon: KeyRound },
{ to: '/settings/files', label: 'Files', icon: FolderSync },
Expand All @@ -65,6 +85,11 @@
const [workspaceOpen, setWorkspaceOpen] = useState(isWorkspaceActive);
const [integrationOpen, setIntegrationOpen] = useState(isIntegrationActive);

const handleNavigate = (path: string) => {
navigate(path);
if (isOpen) onToggle();
};

useEffect(() => {
if (isWorkspaceActive) {
setWorkspaceOpen(true);
Expand All @@ -77,6 +102,14 @@
}
}, [isIntegrationActive]);

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

Check failure on line 111 in web/src/components/Sidebar.tsx

View workflow job for this annotation

GitHub Actions / warden: vercel-react-best-practices

Missing cleanup for global event listener 

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

Check warning on line 111 in web/src/components/Sidebar.tsx

View workflow job for this annotation

GitHub Actions / warden: vercel-react-best-practices

Event listener cleanup missing 

Resize listener added but never removed, causing memory leak. Return cleanup function from useEffect.

Check failure on line 111 in web/src/components/Sidebar.tsx

View workflow job for this annotation

GitHub Actions / warden: code-simplifier

Memory leak: missing event listener cleanup 

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

Check warning on line 111 in web/src/components/Sidebar.tsx

View workflow job for this annotation

GitHub Actions / warden: code-simplifier

Redundant state: sidebarWidth measured from DOM 

sidebarWidth state duplicates CSS-controlled width. Remove state and DOM measurement logic.
Comment on lines +105 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

⚠️ [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

Comment on lines +105 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines +105 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

⚠️ [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

Comment on lines 36 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines +105 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

⚠️ [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

Comment on lines +105 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Comment on lines +105 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

⚠️ [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

Comment on lines 36 to +111
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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


return (
<>
<div
Expand Down Expand Up @@ -119,21 +152,19 @@
<Boxes className="h-4 w-4 text-muted-foreground" />
<span>All Workspaces</span>
</Link>
{workspaces?.map((ws: WorkspaceInfo) => {
{workspaces?.map((ws: WorkspaceInfo, index: number) => {
const wsPath = `/workspaces/${ws.name}`;
const isActive =
location.pathname === wsPath || location.pathname.startsWith(`${wsPath}/`);
return (
<button
key={ws.name}
key={index}
Copy link
Copy Markdown

@github-actions github-actions bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟠 [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

Copy link
Copy Markdown

@sentry sentry bot Feb 8, 2026

Choose a reason for hiding this comment

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

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.

className={cn(
'w-full flex items-center gap-2.5 rounded px-2 py-2 text-sm transition-colors hover:bg-accent group min-h-[44px]',
isActive && 'nav-active'
)}
onClick={() => {
navigate(wsPath);
if (isOpen) onToggle();
}}
style={{ padding: isActive ? '8px 12px' : '8px 8px' }}
onClick={() => handleNavigate(wsPath)}
>
<span
className={cn(
Expand Down
Loading