Skip to content

test: warden react best practices review #157

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 #157

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.

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

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.

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

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.

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


// 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 },
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.

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

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.

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

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.

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

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.

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


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

@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 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.

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