Skip to content

test: warden react best practices review #158

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

Open
gricha wants to merge 2 commits into
base: main
Choose a base branch
from
Open

test: warden react best practices review #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa96735
feat: add workspace count and sidebar improvements
gricha Feb 8, 2026
83ad7f9
chore: set warden to only report high severity findings
gricha Feb 8, 2026
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
4 changes: 4 additions & 0 deletions warden.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ version = 1
[defaults.filters]
ignorePaths = ["docs/**", "**/*.md", "**/*.lock", "**/*.json"]

[defaults.output]
failOn = "high"
commentOn = "high"

[[triggers]]
name = "security-review"
event = "pull_request"
Expand Down
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,6 +32,8 @@
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'],
Expand All @@ -43,6 +45,24 @@
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 },
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 event listener cleanup 

Resize event listener is 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

Missing event listener cleanup causes memory leak 

resize event listener added but never removed - must return cleanup function from useEffect.
Comment on lines +105 to +111
Copy link

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

⚠️ [DH4-NLU] Missing event listener cleanup (high confidence)

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

Suggested fix: Add cleanup function 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 vercel-react-best-practices · high, high confidence

Comment on lines +105 to +111
Copy link

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

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

resize event listener added but never removed - 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 code-simplifier · high, 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

@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 can cause incorrect rendering and state issues when workspaces are added or removed.
Severity: MEDIUM

Suggested Fix

Use a unique and stable identifier from the data for the key prop. The ws.name property was used previously and should be restored. Change key={index} back to 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 component uses the array index as the `key` prop when rendering a
list of workspaces. The list of workspaces is dynamic, as users can create and delete
them. List items also have state-dependent styling based on whether they are active.
When the list is modified (e.g., a workspace is deleted), React will use the index to
reconcile the list, which can lead to incorrect styles being applied to the wrong
workspace items because the state is not correctly mapped to the item.

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