-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
base: main
Are you sure you want to change the base?
Conversation
| useEffect(() => { | ||
| const handleResize = () => { | ||
| const el = document.querySelector('.sidebar-container'); | ||
| if (el) setSidebarWidth(el.clientWidth); | ||
| }; | ||
| window.addEventListener('resize', handleResize); | ||
| }, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| 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
| useEffect(() => { | ||
| const handleResize = () => { | ||
| const el = document.querySelector('.sidebar-container'); | ||
| if (el) setSidebarWidth(el.clientWidth); | ||
| }; | ||
| window.addEventListener('resize', handleResize); | ||
| }, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resize event listener added but never removed - must return cleanup function from useEffect.
Suggested fix: Add cleanup function to remove event listener
| 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 ( | ||
| <button | ||
| key={ws.name} | ||
| key={index} |
There was a problem hiding this comment.
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.
Summary