fix(front): prevent dashboard layout onLayoutChange feedback loop
https://sonarly.com/issue/39408?type=bug Editing a dashboard can crash the page with “Maximum update depth exceeded”, blocking further layout editing in-session. Fix: Implemented a guard in `usePageLayoutHandleLayoutChange` to prevent redundant state writes when `react-grid-layout` emits an `onLayoutChange` payload that is effectively identical to the current tab layout. What changed: - Added `areLayoutsEqual(firstLayouts, secondLayouts)` (JSON structural comparison). - In `handleLayoutChange`, before writing `pageLayoutCurrentLayoutsState`, compare `currentTabLayouts[activeTabId]` against `allLayouts`. - Only call `store.set(pageLayoutCurrentLayoutsState, ...)` when the layout actually changed. Why this fixes the loop: - The feedback loop came from unconditional `store.set` on every `onLayoutChange`, which can feed new props back into grid layout and retrigger `onLayoutChange`. - Skipping no-op writes breaks that self-sustaining render/update cycle for identical layout callbacks. Test added: - `usePageLayoutHandleLayoutChange.test.tsx`: new case `"should not rewrite tab layouts when new layout is identical"` asserts that a second identical `handleLayoutChange` call keeps the same stored layout reference for the tab (no rewrite). Validation attempted: - Ran: - `npx jest packages/twenty-front/src/modules/page-layout/hooks/__tests__/usePageLayoutHandleLayoutChange.test.tsx --config=packages/twenty-front/jest.config.mjs` - It failed due missing `ts-jest` in this environment, so I could not execute tests locally. Authored by Sonarly by autonomous analysis (run 44884).
This commit is contained in:
+46
@@ -129,4 +129,50 @@ describe('usePageLayoutHandleLayoutChange', () => {
|
||||
|
||||
expect(Object.keys(result.current.layouts)).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('should not rewrite tab layouts when new layout is identical', () => {
|
||||
const { result } = renderHook(
|
||||
() => ({
|
||||
handler: usePageLayoutHandleLayoutChange({
|
||||
pageLayoutId: PAGE_LAYOUT_TEST_INSTANCE_ID,
|
||||
tabListInstanceId: getTabListInstanceIdFromPageLayoutId(
|
||||
PAGE_LAYOUT_TEST_INSTANCE_ID,
|
||||
),
|
||||
}),
|
||||
layouts: useAtomComponentStateValue(
|
||||
pageLayoutCurrentLayoutsComponentState,
|
||||
PAGE_LAYOUT_TEST_INSTANCE_ID,
|
||||
),
|
||||
setActiveTabId: useSetAtom(
|
||||
activeTabIdComponentState.atomFamily({
|
||||
instanceId: `${PAGE_LAYOUT_TEST_INSTANCE_ID}-tab-list`,
|
||||
}),
|
||||
),
|
||||
}),
|
||||
{
|
||||
wrapper: PageLayoutTestWrapper,
|
||||
},
|
||||
);
|
||||
|
||||
act(() => {
|
||||
result.current.setActiveTabId('tab-1');
|
||||
});
|
||||
|
||||
const newLayouts = {
|
||||
desktop: [{ i: 'widget-1', x: 1, y: 0, w: 2, h: 2 }],
|
||||
mobile: [{ i: 'widget-1', x: 0, y: 0, w: 1, h: 2 }],
|
||||
};
|
||||
|
||||
act(() => {
|
||||
result.current.handler.handleLayoutChange([], newLayouts);
|
||||
});
|
||||
|
||||
const firstStoredLayouts = result.current.layouts['tab-1'];
|
||||
|
||||
act(() => {
|
||||
result.current.handler.handleLayoutChange([], newLayouts);
|
||||
});
|
||||
|
||||
expect(result.current.layouts['tab-1']).toBe(firstStoredLayouts);
|
||||
});
|
||||
});
|
||||
|
||||
+15
-4
@@ -10,6 +10,10 @@ import { useCallback } from 'react';
|
||||
import { type Layout, type Layouts } from 'react-grid-layout';
|
||||
import { isDefined } from 'twenty-shared/utils';
|
||||
|
||||
const areLayoutsEqual = (firstLayouts: Layouts, secondLayouts: Layouts) => {
|
||||
return JSON.stringify(firstLayouts) === JSON.stringify(secondLayouts);
|
||||
};
|
||||
|
||||
export const usePageLayoutHandleLayoutChange = ({
|
||||
pageLayoutId: pageLayoutIdFromProps,
|
||||
tabListInstanceId,
|
||||
@@ -45,11 +49,18 @@ export const usePageLayoutHandleLayoutChange = ({
|
||||
if (!isDefined(activeTabId)) return;
|
||||
|
||||
const currentTabLayouts = store.get(pageLayoutCurrentLayoutsState);
|
||||
const activeTabLayouts = currentTabLayouts[activeTabId];
|
||||
|
||||
store.set(pageLayoutCurrentLayoutsState, {
|
||||
...currentTabLayouts,
|
||||
[activeTabId]: structuredClone(allLayouts),
|
||||
});
|
||||
const hasLayoutChanged =
|
||||
!isDefined(activeTabLayouts) ||
|
||||
!areLayoutsEqual(activeTabLayouts, allLayouts);
|
||||
|
||||
if (hasLayoutChanged) {
|
||||
store.set(pageLayoutCurrentLayoutsState, {
|
||||
...currentTabLayouts,
|
||||
[activeTabId]: structuredClone(allLayouts),
|
||||
});
|
||||
}
|
||||
|
||||
const pageLayoutDraft = store.get(pageLayoutDraftState);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user