fix: new revert change #18
Conversation
Summary of ChangesHello @rajasekhar524, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on reverting a previous change related to green flag click handling in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request appears to revert several previous changes, some of which were critical fixes for performance and stability. While some of the changes are simple code cleanup, there are several high-risk reversions that need to be addressed. Specifically, a change in src/containers/blocks.jsx re-introduces code that was known to cause infinite renders. Additionally, setTimeout wrappers, which were in place to prevent illegal state updates during React's render cycle, have been removed in multiple components. The production build script has also been modified riesgosamente, potentially resulting in a non-optimized development build. I've detailed these issues in the comments below.
| // REMOVED: this.handleGreenbuttonClick() - This was causing infinite renders | ||
| // The handleGreenbuttonClick was updating Redux state on every workspace metric change, | ||
| // which triggered re-renders that caused more workspace metric changes. | ||
| this.handleGreenbuttonClick() |
There was a problem hiding this comment.
This line re-introduces a call to this.handleGreenbuttonClick() which was previously commented out with a note that it causes infinite renders. The original comment stated: "The handleGreenbuttonClick was updating Redux state on every workspace metric change, which triggered re-renders that caused more workspace metric changes." Restoring this call is highly likely to re-introduce this critical performance bug.
| this.handleGreenbuttonClick() | |
| // REMOVED: this.handleGreenbuttonClick() - This was causing infinite renders | |
| // The handleGreenbuttonClick was updating Redux state on every workspace metric change, | |
| // which triggered re-renders that caused more workspace metric changes. |
| "scripts": { | ||
| "build": "npm run clean && NODE_ENV=production webpack && cp -r ./build/ ../public/scratch", | ||
| "build:dev": "npm run clean && webpack && cp -r ./build/ ../public/scratch", | ||
| "build": "npm run clean && webpack && cp -r ./build/ ../public/scratch", |
There was a problem hiding this comment.
The build script no longer sets NODE_ENV=production. This is risky as it could result in a non-optimized development build being deployed to production. Development builds are larger and less performant. It's safer to explicitly set the environment for the production build. The build:dev script was also removed, which was likely the intended script for development builds. Consider restoring the previous configuration for both scripts.
| "build": "npm run clean && webpack && cp -r ./build/ ../public/scratch", | |
| "build": "npm run clean && NODE_ENV=production webpack && cp -r ./build/ ../public/scratch", | |
| "build:dev": "npm run clean && webpack && cp -r ./build/ ../public/scratch", |
| setTimeout(() => { | ||
| props.setProjectName(message) | ||
| }, 0) | ||
| props.setProjectName(message) |
There was a problem hiding this comment.
The setTimeout wrapper around this state update has been removed. The previous implementation included it to defer the update and avoid a common React warning: "Cannot update a component while rendering a different component". Removing this setTimeout is risky and may reintroduce this instability.
setTimeout(() => {
props.setProjectName(message)
}, 0)| useEffect(() => { | ||
| if (componentProps.costumeURL) { | ||
| // Use setTimeout to defer the state update to avoid updating during render | ||
| const timeoutId = setTimeout(() => { | ||
| setCostumeClickedState(componentProps.costumeURL) | ||
| }, 0) | ||
|
|
||
| return () => clearTimeout(timeoutId) | ||
| } | ||
| }, [setCostumeClickedState, componentProps.costumeURL]) | ||
| props.setCostumeClickedState(props.costumeURL) | ||
| }, []) |
There was a problem hiding this comment.
This useEffect has two potential issues:
- The
setTimeoutwhich was used to defer state updates and prevent render-cycle warnings has been removed. This could reintroduce instability. - The dependency array has been changed to
[], meaning this effect will only run once when the component mounts. The previous implementation includedcostumeURLin the dependencies, ensuring the effect would re-run if the costume URL changed. Ifprops.costumeURLcan change during the component's lifetime, this is now a bug, as the state will not be updated.
useEffect(() => {
if (props.costumeURL) {
// Use setTimeout to defer the state update to avoid updating during render
const timeoutId = setTimeout(() => {
props.setCostumeClickedState(props.costumeURL)
}, 0)
return () => clearTimeout(timeoutId)
}
}, [props.setCostumeClickedState, props.costumeURL])| <svg xmlns="http://www.w3.org/2000/svg" fill="none" stroke='#801d80' viewBox="0 0 24 24" stroke-width="1.5" class="size-2"> | ||
| <path stroke-linecap="round" stroke-linejoin="round" d="M16.023 9.348h4.992v-.001M2.985 19.644v-4.992m0 0h4.992m-4.993 0 3.181 3.183a8.25 8.25 0 0 0 13.803-3.7M4.031 9.865a8.25 8.25 0 0 1 13.803-3.7l3.181 3.182m0-4.991v4.99" /> |
There was a problem hiding this comment.
In JSX, you should use className instead of class for CSS classes. Additionally, React convention is to use camelCase for SVG attributes (e.g., strokeWidth, strokeLinecap, strokeLinejoin) instead of kebab-case. While the current code might work in some browsers, it will produce warnings in the console and goes against standard React practices.
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" stroke='#801d80' viewBox="0 0 24 24" stroke-width="1.5" class="size-2"> | |
| <path stroke-linecap="round" stroke-linejoin="round" d="M16.023 9.348h4.992v-.001M2.985 19.644v-4.992m0 0h4.992m-4.993 0 3.181 3.183a8.25 8.25 0 0 0 13.803-3.7M4.031 9.865a8.25 8.25 0 0 1 13.803-3.7l3.181 3.182m0-4.991v4.99" /> | |
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" stroke='#801d80' viewBox="0 0 24 24" strokeWidth="1.5" className="size-2"> | |
| <path strokeLinecap="round" strokeLinejoin="round" d="M16.023 9.348h4.992v-.001M2.985 19.644v-4.992m0 0h4.992m-4.993 0 3.181 3.183a8.25 8.25 0 0 0 13.803-3.7M4.031 9.865a8.25 8.25 0 0 1 13.803-3.7l3.181 3.182m0-4.991v4.99" /> |
1 participant
Resolves
What Github issue does this resolve (please include link)?
Proposed Changes
Describe what this Pull Request does
Reason for Changes
Explain why these changes should be made
Test Coverage
Please show how you have added tests to cover your changes
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet