Skip to content

Improve state management [SYNTH-263]#1433

Open
0xda157 wants to merge 8 commits into
devfrom
dionkud/do-not-mutate-react
Open

Improve state management [SYNTH-263]#1433
0xda157 wants to merge 8 commits into
devfrom
dionkud/do-not-mutate-react

Conversation

@0xda157

@0xda157 0xda157 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Task

SYNTH-263

Symptom

many of our components include useReducers purely to avoid manage state in the correct react way

Solution

this pr improve the state handling in many locations, primarily by copying the state using their set function instead of directly mutating them.

Note

There are few more places with very poor state management, but I've deemed them too difficult to improve in this pr. Ideally they will be addressed later.

Verification

Playwright tests pass, and the UI seems to work as expected.


Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

@0xda157
0xda157 requested a review from rutmanz July 22, 2026 17:02
@0xda157
0xda157 requested review from a team as code owners July 22, 2026 17:02
@0xda157
0xda157 force-pushed the dionkud/do-not-mutate-react branch from 7da54be to 5134e57 Compare July 22, 2026 17:13
@0xda157
0xda157 force-pushed the dionkud/do-not-mutate-react branch from 870aaf6 to bea3dff Compare July 22, 2026 20:55
Comment on lines +134 to +137
const [pref, setPref] = useState(PreferencesSystem.getUserPreference(preference))
useEffect(() => {
PreferencesSystem.setUserPreference(preference, pref)
}, [pref])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will fire a preferenceChanged event for every preference on mount and twice in dev strictmode

instead, I would suggest

<Checkbox {...props} label={label} checked={pref} onClick={
setPref(val)
PreferencesSystem.setUserPreferences(preference, val)
} />

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This will fire a preferenceChanged event for every preference on mount and twice in dev strictmode

why is this an issue?

instead, I would suggest

<Checkbox {...props} label={label} checked={pref} onClick={
setPref(val)
PreferencesSystem.setUserPreferences(preference, val)
} />

this is a very strong react anti-pattern. If setPref gets called somewhere else, after this change, it will lead to the UI being in an invalid state.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with @0xda157; there's not much of a cost to the event being fired repeatedly since we have relatively few listeners and preferences, and it avoids ending up with an invalid UI state

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So we are reading from the preferences and setting the preferences...

There are 13 preference events being triggered and it seems that 8 do not have listeners:
ReportAnalytics, SubsystemGravity, RenderSceneTag, RenderScoreboard, ShowViewCube

ReportAnalytics & SubsystemGravity might be problematic:
-> ReportAnalytics calls sendMetaData() [eventually] -> sending to google analytics
-> calls GetMotorSettings() and rewrites 2 torque limits on a constraint

@PepperLola @0xda157

@0xda157
0xda157 force-pushed the dionkud/do-not-mutate-react branch from 8f8052e to 68c28c7 Compare July 24, 2026 18:10
@0xda157
0xda157 requested a review from Dhruv-0-Arora July 24, 2026 21:13
@PepperLola PepperLola added ui/ux Relating to user interface, or in general, user experience development Related to the development process (workflows, dev-facing improvements, etc.) labels Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development Related to the development process (workflows, dev-facing improvements, etc.) ui/ux Relating to user interface, or in general, user experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants