Skip to content

fix: new revert change #18

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
wants to merge 2 commits into
base: qubits_develop
Choose a base branch
from
Open

fix: new revert change #18

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
8789747
head revert
rajasekhar524 Sep 26, 2025
1b88a3b
revert new branch
rajasekhar524 Sep 26, 2025
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
228 changes: 0 additions & 228 deletions debug-render-analysis.js
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
"default": "./src/index.js"
},
"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",
Copy link

Choose a reason for hiding this comment

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

high

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.

Suggested change
"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",

"clean": "rimraf ./build ./dist",
"deploy": "touch build/.nojekyll && gh-pages -t -d build -m \"[skip ci] Build for $(git log --pretty=format:%H -n1)\"",
"prepublish": "node scripts/prepublish.mjs",
Expand Down
3 changes: 1 addition & 2 deletions src/components/controls/controls.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ const Controls = function (props) {
const {
active,
className,
costumeURLFax,
intl,
onGreenFlagClick,
onStopAllClick,
Expand Down Expand Up @@ -124,4 +123,4 @@ const mapStateToProps = (state) => ({
flagClicked: state.scratchGui.vmStatus.flagClicked,
})

export default injectIntl(connect(mapStateToProps, () => ({}))(Controls))
export default injectIntl(connect(mapStateToProps, null)(Controls))
4 changes: 2 additions & 2 deletions src/components/customi-cons/reload.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import styles from './reload.css'
function Reload() {
return (
<div className={styles.reloadContainer }>
<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" />
<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" />
Comment on lines +7 to +8
Copy link

Choose a reason for hiding this comment

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

medium

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.

Suggested change
<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" />

</svg>
</div>
)
Expand Down
16 changes: 3 additions & 13 deletions src/components/gui/gui.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,6 @@ const GUIComponent = (props) => {
onTelemetryModalCancel,
onTelemetryModalOptIn,
onTelemetryModalOptOut,
setPositionModal,
setProjectName,
setSpriteClickedState,
addNotification,
removeNotification,
setCurrentLayout,
showComingSoon,
soundsTabVisible,
stageSizeMode,
Expand Down Expand Up @@ -193,10 +187,7 @@ const GUIComponent = (props) => {
messenger,
methods: {
getScratchState(message) {
// Use setTimeout to defer the state update to avoid updating during render
setTimeout(() => {
props.setProjectName(message)
}, 0)
props.setProjectName(message)
Copy link

Choose a reason for hiding this comment

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

high

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)

},
},
timeout: 15000,
Expand All @@ -219,12 +210,11 @@ const GUIComponent = (props) => {
}
}, [])


useEffect(() => {
if (currentLayout === 'myprojects') {
setPositionModal(true)
props.setPositionModal(true)
}
}, [currentLayout, setPositionModal])
}, [currentLayout])

useEffect(() => {
if (remote && currentLayout === 'studentChallenge') {
Expand Down
6 changes: 1 addition & 5 deletions src/components/menu-bar/menu-bar-gui-sub.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,10 +649,6 @@ class MenuBarGuiSub extends React.Component {
}
}
render() {
const {
greenFlagClicked,
...componentProps
} = this.props

const newProjectMessage = (
<FormattedMessage
Expand All @@ -662,7 +658,7 @@ class MenuBarGuiSub extends React.Component {
/>
)
return (
<Box className={classNames(componentProps.className, styles.menuBar)}>
<Box className={classNames(this.props.className, styles.menuBar)}>
{this.props.canManageFiles && (
<div
className={classNames(styles.menuBarItem, styles.hoverable, {
Expand Down
16 changes: 2 additions & 14 deletions src/components/sprite-selector-item/sprite-selector-item.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,10 @@ import { setCostumeClickedState } from './../../reducers/vm-status.js'
let contextMenuId = 0

const SpriteSelectorItem = (props) => {
const {
costumeURLFax,
setCostumeClickedState,
...componentProps
} = props

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)
}, [])
Comment on lines 17 to +19
Copy link

Choose a reason for hiding this comment

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

high

This useEffect has two potential issues:

  1. The setTimeout which was used to defer state updates and prevent render-cycle warnings has been removed. This could reintroduce instability.
  2. The dependency array has been changed to [], meaning this effect will only run once when the component mounts. The previous implementation included costumeURL in the dependencies, ensuring the effect would re-run if the costume URL changed. If props.costumeURL can 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])


return (
<ContextMenuTrigger
Expand Down
2 changes: 1 addition & 1 deletion src/components/stage-wrapper/stage-wrapper.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import styles from './stage-wrapper.css'


const StageWrapperComponent = function (props) {
const { isFullScreen, isRtl, isRendererSupported, loading, stageSize, vm, flagClicked, currentLayout, setFlagClickedState } = props
const { isFullScreen, isRtl, isRendererSupported, loading, stageSize, vm, flagClicked, currentLayout } = props
return (
<Box
className={classNames(styles.stageWrapper, { [styles.fullScreen]: isFullScreen })}
Expand Down
1 change: 0 additions & 1 deletion src/components/stage/stage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ const StageComponent = (props) => {
colorInfo,
micIndicator,
question,
setFlagClickedState,
stageSize,
useEditorDragStyle,
onDeactivateColorPicker,
Expand Down
8 changes: 1 addition & 7 deletions src/containers/blocks.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,7 @@ class Blocks extends React.Component {
})
}, 0)
}
// 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()
Copy link

Choose a reason for hiding this comment

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

critical

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.

Suggested change
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.

}
onScriptGlowOn(data) {
this.workspace.glowStack(data.id, true)
Expand Down Expand Up @@ -576,14 +574,10 @@ class Blocks extends React.Component {
/* eslint-disable no-unused-vars */
const {
anyModalVisible,
autoSave,
canUseCloud,
customProceduresVisible,
extensionLibraryVisible,
flagClicked,
options,
setAutoSaveState,
setFlagClickedState,
stageSize,
vm,
isRtl,
Expand Down
Loading