Fix unnecessary rerendering of side content in mobile workspace #2338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is so that we can have more granular memoization to reduce unnecessary re-rendering. Otherwise, the hidden '_owner' attribute on the autorun React element object keeps changing, causing the memoization of MobileSideContent to not work.
ianyong
commented
Feb 1, 2023
| @@ -512,16 +549,23 @@ const Playground: React.FC<PlaygroundProps> = ({ workspaceLocation = 'playground | |||
|
|
|||
| const { handleEditorValueChange } = props; | |||
|
|
|||
| // No point memoing this, it uses props.editorValue | |||
There was a problem hiding this comment.
This turned out to be untrue...
ianyong
commented
Feb 1, 2023
src/pages/playground/Playground.tsx
Outdated
Comment on lines
552
to
555
| const getEditorValue = React.useCallback( | ||
| () => store.getState().workspaces.playground.editorTabs[0].value, | ||
| [store] | ||
| ); |
There was a problem hiding this comment.
We do this so that we only access the editor value when the session button is pressed, instead of constantly re-rendering the session button whenever the editor value changes. As a result, we need the store object which we retrieve using the useStore hook. This hook does not listen to changes (as compared to the useSelector hook which does and will force re-renders every time the listened value updates).
Pull Request Test Coverage Report for Build 4090243912Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
RichDom2185
reviewed
Feb 2, 2023
Currently, the 'Playground' component is also used by the SICP workspace. Co-authored-by: Richard Dominick <[email protected]>
ianyong
force-pushed
the
fix-unnecessary-rerendering-of-mobile-side-content
branch
from
5386c59 to
4fecd8c
Compare
ianyong
deleted the
fix-unnecessary-rerendering-of-mobile-side-content
branch
RichDom2185
added a commit
to NUS-CS1101S/cadet-frontend
that referenced
this pull request
Jul 15, 2023
…ce-academy#2338) * Memoize SideContent & MobileSideContent only when exporting as default * Update test snapshots * Memoize onChange callback function * Memoize session buttons in Playground * Refactor session button to only access editor value when clicked * Memoize callbacks passed into autorun button * Specify autorun button props manually instead of using Lodash's pick This is so that we can have more granular memoization to reduce unnecessary re-rendering. Otherwise, the hidden '_owner' attribute on the autorun React element object keeps changing, causing the memoization of MobileSideContent to not work. * Make use of 'workspaceLocation' in 'getEditorValue' Currently, the 'Playground' component is also used by the SICP workspace. Co-authored-by: Richard Dominick <[email protected]> --------- Co-authored-by: Richard Dominick <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Played whack-a-mole with the React profiler until everything that is causing unnecessary re-rending of the side content tabs in the mobile view is memoized in some form or another. Note that unnecessary re-rendering here is only in the context of typing in the editor. The side content in the mobile view will definitely still re-render more than is necessary (i.e., more than the desktop view) by virtue of the React component hierarchy described in the paragraph below.
The main difficulty encountered here is that in the desktop workspace, the control bar is not nested in the side content component. As such, both of these components can re-render separately. In the mobile workspace however, the control bar is a child of the side content component. Whenever the control bar has to re-render, so does the side content. An alternative to all of these memoization would be to redesign the mobile workspace such that the control bar is not nested inside the side content component. This would also help prevent future changes from reintroducing this unnecessary re-render issue. However, this is also not an easy refactor to carry out.
Also implements @RichDom2185's suggestion to memoize components when exporting as default.
See each individual commit for the context behind every change.
Fixes #2332. Related to #2331.
Type of change
How to test
Replicate the steps in the linked issue.