Skip to content
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

Fix DockLayout.updateTabCache #214

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Shravankumarkarnati
Copy link

@Shravankumarkarnati Shravankumarkarnati commented Jul 22, 2023

Fixes #198, #195

This seems to be a problem with useEffect running twice in React 18 StrictMode.

Fixed it by adding an equality check on current cache and given children to update.
Only update the cache (and forceUpdate) if they are not same.

@Shravankumarkarnati
Copy link
Author

@rinick @slavamuravey does this look right to you?

@zelongjiang123
Copy link

Can someone please fix this bug?

@yepw
Copy link

yepw commented Aug 11, 2023

This seems like a known issue. It's frustrating that the layout crashes occasionally. If this pull request fixes the problem, it would be great to merge it to the main branch.

@@ -161,6 +161,9 @@ class DockPortalManager extends React.PureComponent<LayoutProps, LayoutState> {
updateTabCache(id: string, children: React.ReactNode): void {
let cache = this._caches.get(id);
if (cache) {
if (Object.is(cache.portal?.children, children)) {
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, add semicolon.

Choose a reason for hiding this comment

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

added!

@garyvh2
Copy link

garyvh2 commented Sep 12, 2023

Applied temporary patch-package with the fix the problem is still occurring but less frequently.

Here's an example: https://codesandbox.io/s/cycle-update-bug-vdphjp?file=/src/App.tsx

Drag a bunch of Draggable elements into the layout as floating windows, it crashes after a bit.

This might be related to the onLayoutChange on controlled layouts, as removing that and layout definition (leaving defaultLayout and loadTab only) solves the problem altogether.

@Shravankumarkarnati
Copy link
Author

Shravankumarkarnati commented Sep 14, 2023

@garyvh2 looks like the tabs in your example are unstable. Their randomly generated id keeps changing on every update and so the cache is updating.
If you don't have stable ids, you can leave it to rc-dock. I believe rc-dock creates and re-uses tab ids on updates.

@garyvh2
Copy link

garyvh2 commented Sep 19, 2023

I've updated the solution in this case removing the uuid. I tried to let rc-dock handle the id on it's own, but it seems it's not possible, if not provided then the id is set as undefined.

I've updated the example with a simple count on the outside, which is set on drag n drop, and also used the prior id on loadTab, but the problem is still present.

@denspi
Copy link

denspi commented Nov 24, 2023

Can someone approve this? We get similar error for React 18 floating tabs when window is resized and it crashes the app.

react-dom.development.js:27292 Uncaught Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
at checkForNestedUpdates (react-dom.development.js:27292:1)
at scheduleUpdateOnFiber (react-dom.development.js:25475:1)
at Object.enqueueForceUpdate (react-dom.development.js:14120:1)
at Component.forceUpdate (react.development.js:373:1)
at DockLayout.updateTabCache (DockLayout.js:74:1)
at DockTabPane.updateCache (DockTabPane.js:24:1)
at DockTabPane.componentDidUpdate (DockTabPane.js:63:1)
at commitLayoutEffectOnFiber (react-dom.development.js:23333:1)
at commitLayoutMountEffects_complete (react-dom.development.js:24688:1)
at commitLayoutEffects_begin (react-dom.development.js:24674:1)

@maulep1
Copy link

maulep1 commented Nov 28, 2023

Hello @Shravankumarkarnati could you please approve this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants