From ba65f18678888b1477b203ed07950105f2248f10 Mon Sep 17 00:00:00 2001 From: Aleksandar Petrov <8142643+aleks-p@users.noreply.github.com> Date: Tue, 2 Dec 2025 10:02:10 -0400 Subject: [PATCH] Fix excessive re-renders, dragging multiple panels --- .../components/ExploreMapCanvas.tsx | 4 +- .../components/ExploreMapPanelContainer.tsx | 94 ++++++++++++------- .../components/ExploreMapPanelContent.tsx | 56 ++++++----- public/app/features/explore-map/crdt/state.ts | 20 +++- .../explore-map/hooks/usePanelStateSync.ts | 49 ++++------ .../explore-map/operations/validators.ts | 2 +- .../features/explore-map/state/crdtSlice.ts | 28 +++++- .../features/explore-map/state/selectors.ts | 39 +++++++- 8 files changed, 194 insertions(+), 98 deletions(-) diff --git a/public/app/features/explore-map/components/ExploreMapCanvas.tsx b/public/app/features/explore-map/components/ExploreMapCanvas.tsx index e2f6f83d412..a460e93816f 100644 --- a/public/app/features/explore-map/components/ExploreMapCanvas.tsx +++ b/public/app/features/explore-map/components/ExploreMapCanvas.tsx @@ -251,9 +251,7 @@ export function ExploreMapCanvas() { tabIndex={0} > {Object.values(panels).map((panel) => { - // Use remoteVersion in the key to force re-render when remote state changes - // remoteVersion only increments on remote updates, so local edits won't trigger re-render - return ; + return ; })} {Object.values(cursors).map((cursor) => ( diff --git a/public/app/features/explore-map/components/ExploreMapPanelContainer.tsx b/public/app/features/explore-map/components/ExploreMapPanelContainer.tsx index 5b3c00a8394..5998c8d02d1 100644 --- a/public/app/features/explore-map/components/ExploreMapPanelContainer.tsx +++ b/public/app/features/explore-map/components/ExploreMapPanelContainer.tsx @@ -1,5 +1,5 @@ import { css, cx } from '@emotion/css'; -import { useCallback, useRef, useState } from 'react'; +import React, { useCallback, useRef, useState } from 'react'; import { Rnd, RndDragCallback, RndResizeCallback } from 'react-rnd'; import { GrafanaTheme2 } from '@grafana/data'; @@ -10,9 +10,11 @@ import { useDispatch, useSelector } from 'app/types/store'; import { splitClose } from '../../explore/state/main'; import { bringPanelToFront, + clearActiveDrag, duplicatePanel, removePanel, selectPanel, + setActiveDrag, updateMultiplePanelPositions, updatePanelPosition, updatePanelSize, @@ -26,7 +28,7 @@ interface ExploreMapPanelContainerProps { panel: ExploreMapPanel; } -export function ExploreMapPanelContainer({ panel }: ExploreMapPanelContainerProps) { +function ExploreMapPanelContainerComponent({ panel }: ExploreMapPanelContainerProps) { const styles = useStyles2(getStyles); const dispatch = useDispatch(); const rndRef = useRef(null); @@ -36,6 +38,19 @@ export function ExploreMapPanelContainer({ panel }: ExploreMapPanelContainerProp const viewport = useSelector((state) => selectViewport(state.exploreMapCRDT)); const isSelected = selectedPanelIds.includes(panel.id); + // Get the active drag info from a parent context (if any panel is being dragged) + const activeDragInfo = useSelector((state) => state.exploreMapCRDT.local.activeDrag); + + // Calculate effective position considering active drag + let effectiveX = panel.position.x; + let effectiveY = panel.position.y; + + // If another panel in the selection is being dragged, apply the offset to this panel + if (activeDragInfo && activeDragInfo.draggedPanelId !== panel.id && isSelected) { + effectiveX += activeDragInfo.deltaX; + effectiveY += activeDragInfo.deltaY; + } + const handleDragStart: RndDragCallback = useCallback( (_e, data) => { setDragStartPos({ x: data.x, y: data.y }); @@ -52,23 +67,24 @@ export function ExploreMapPanelContainer({ panel }: ExploreMapPanelContainerProp const deltaX = data.x - dragStartPos.x; const deltaY = data.y - dragStartPos.y; - // If this panel is selected and there are multiple selections, move all others + // If dragging multiple panels, store the drag offset in local state + // This will cause other panels to visually move without CRDT operations if (isSelected && selectedPanelIds.length > 1) { - dispatch( - updateMultiplePanelPositions({ - panelId: panel.id, - deltaX, - deltaY, - }) - ); - setDragStartPos({ x: data.x, y: data.y }); + dispatch(setActiveDrag({ + draggedPanelId: panel.id, + deltaX: data.x - panel.position.x, + deltaY: data.y - panel.position.y, + })); } }, - [dispatch, panel.id, dragStartPos, isSelected, selectedPanelIds.length] + [dispatch, panel.id, panel.position.x, panel.position.y, dragStartPos, isSelected, selectedPanelIds.length] ); const handleDragStop: RndDragCallback = useCallback( (_e, data) => { + // Clear the active drag state + dispatch(clearActiveDrag()); + if (dragStartPos) { const deltaX = data.x - dragStartPos.x; const deltaY = data.y - dragStartPos.y; @@ -77,9 +93,9 @@ export function ExploreMapPanelContainer({ panel }: ExploreMapPanelContainerProp const hasMoved = Math.abs(deltaX) > 0.5 || Math.abs(deltaY) > 0.5; if (hasMoved) { - // Final position update for all panels if (isSelected && selectedPanelIds.length > 1) { - // Update other panels with the remaining delta + // Multi-panel drag: update all selected panels with the delta + // This creates CRDT operations that will be broadcast dispatch( updateMultiplePanelPositions({ panelId: panel.id, @@ -87,24 +103,16 @@ export function ExploreMapPanelContainer({ panel }: ExploreMapPanelContainerProp deltaY, }) ); - // Update the dragged panel's position in Redux - dispatch( - updatePanelPosition({ - panelId: panel.id, - x: data.x, - y: data.y, - }) - ); - } else { - // Single panel drag - dispatch( - updatePanelPosition({ - panelId: panel.id, - x: data.x, - y: data.y, - }) - ); } + + // Always update the dragged panel's final position + dispatch( + updatePanelPosition({ + panelId: panel.id, + x: data.x, + y: data.y, + }) + ); } } setDragStartPos(null); @@ -185,7 +193,7 @@ export function ExploreMapPanelContainer({ panel }: ExploreMapPanelContainerProp return ( @@ -237,6 +248,25 @@ export function ExploreMapPanelContainer({ panel }: ExploreMapPanelContainerProp ); } +export const ExploreMapPanelContainer = React.memo(ExploreMapPanelContainerComponent, (prevProps, nextProps) => { + const prev = prevProps.panel; + const next = nextProps.panel; + + // Only re-render if these specific properties change + // This prevents re-renders when unrelated panels update + return ( + prev.id === next.id && + prev.position.x === next.position.x && + prev.position.y === next.position.y && + prev.position.width === next.position.width && + prev.position.height === next.position.height && + prev.position.zIndex === next.position.zIndex && + prev.remoteVersion === next.remoteVersion && + prev.exploreId === next.exploreId && + prev.mode === next.mode + ); +}); + const getStyles = (theme: GrafanaTheme2) => { return { panelContainer: css({ diff --git a/public/app/features/explore-map/components/ExploreMapPanelContent.tsx b/public/app/features/explore-map/components/ExploreMapPanelContent.tsx index 385ef5dce55..a1204f54b18 100644 --- a/public/app/features/explore-map/components/ExploreMapPanelContent.tsx +++ b/public/app/features/explore-map/components/ExploreMapPanelContent.tsx @@ -1,10 +1,10 @@ import { css } from '@emotion/css'; -import { useEffect, useMemo, useState } from 'react'; +import React, { useEffect, useMemo, useState } from 'react'; import { EventBusSrv, GrafanaTheme2 } from '@grafana/data'; import { Trans } from '@grafana/i18n'; import { useStyles2 } from '@grafana/ui'; -import { useDispatch, useSelector } from 'app/types/store'; +import { useDispatch } from 'app/types/store'; import { ExplorePaneContainer } from '../../explore/ExplorePaneContainer'; import { DEFAULT_RANGE } from '../../explore/state/constants'; @@ -12,7 +12,6 @@ import { initializeExplore } from '../../explore/state/explorePane'; import { usePanelStateSync } from '../hooks/usePanelStateSync'; // import { useExploreStateReceiver } from '../hooks/useExploreStateReceiver'; // import { useExploreStateSync } from '../hooks/useExploreStateSync'; -import { selectPanels } from '../state/selectors'; import { ExploreMapLogsDrilldownPanel } from './Drilldown/ExploreMapLogsDrilldownPanel'; import { ExploreMapMetricsDrilldownPanel } from './Drilldown/ExploreMapMetricsDrilldownPanel'; @@ -24,6 +23,9 @@ interface ExploreMapPanelContentProps { exploreId: string; width: number; height: number; + remoteVersion?: number; + mode?: 'explore' | 'traces-drilldown' | 'metrics-drilldown' | 'profiles-drilldown' | 'logs-drilldown'; + exploreState?: any; } @@ -83,7 +85,7 @@ function patchGetBoundingClientRect() { isPatched = true; } -export function ExploreMapPanelContent({ panelId, exploreId, width, height }: ExploreMapPanelContentProps) { +export const ExploreMapPanelContent = React.memo(function ExploreMapPanelContent({ panelId, exploreId, width, height, remoteVersion, mode, exploreState }: ExploreMapPanelContentProps) { const styles = useStyles2(getStyles); const dispatch = useDispatch(); const [isInitialized, setIsInitialized] = useState(false); @@ -117,19 +119,13 @@ export function ExploreMapPanelContent({ panelId, exploreId, width, height }: Ex patchGetBoundingClientRect(); }, []); - // Get panel from CRDT state (which has the latest exploreState) - const panel = useSelector((state) => { - const panels = selectPanels(state.exploreMapCRDT); - return panels[panelId]; - }); - // Initialize Explore pane on mount (only for standard Explore panels) useEffect(() => { if ( - panel?.mode === 'traces-drilldown' || - panel?.mode === 'metrics-drilldown' || - panel?.mode === 'profiles-drilldown' || - panel?.mode === 'logs-drilldown' + mode === 'traces-drilldown' || + mode === 'metrics-drilldown' || + mode === 'profiles-drilldown' || + mode === 'logs-drilldown' ) { // Drilldown panels don't need Explore initialization setIsInitialized(true); @@ -138,9 +134,7 @@ export function ExploreMapPanelContent({ panelId, exploreId, width, height }: Ex const initializePane = async () => { // Use saved state if available, otherwise defaults - const savedState = panel?.exploreState; - - console.log('[ExploreMapPanelContent] Initializing with saved state:', savedState); + const savedState = exploreState; await dispatch( initializeExplore({ @@ -162,21 +156,21 @@ export function ExploreMapPanelContent({ panelId, exploreId, width, height }: Ex eventBus.removeAllListeners(); }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [dispatch, exploreId, eventBus, panel?.exploreState, panel?.mode]); + }, [dispatch, exploreId, eventBus, exploreState, mode]); - if (panel?.mode === 'traces-drilldown') { + if (mode === 'traces-drilldown') { return ; } - if (panel?.mode === 'metrics-drilldown') { + if (mode === 'metrics-drilldown') { return ; } - if (panel?.mode === 'profiles-drilldown') { + if (mode === 'profiles-drilldown') { return ; } - if (panel?.mode === 'logs-drilldown') { + if (mode === 'logs-drilldown') { return ; } @@ -202,7 +196,23 @@ export function ExploreMapPanelContent({ panelId, exploreId, width, height }: Ex ); -} +}, (prevProps, nextProps) => { + // Only re-render if width, height, remoteVersion, or mode changes + // This prevents re-renders when only position changes (local drag operations) + // + // NOTE: We deliberately exclude exploreState from comparison because: + // 1. It's only used during initialization (mount) + // 2. After initialization, the component manages state via Redux + // 3. exploreState is an object that gets new references even when data is the same + return ( + prevProps.panelId === nextProps.panelId && + prevProps.exploreId === nextProps.exploreId && + prevProps.width === nextProps.width && + prevProps.height === nextProps.height && + prevProps.remoteVersion === nextProps.remoteVersion && + prevProps.mode === nextProps.mode + ); +}); const getStyles = (theme: GrafanaTheme2) => { return { diff --git a/public/app/features/explore-map/crdt/state.ts b/public/app/features/explore-map/crdt/state.ts index f4bf6eba081..184d6737e88 100644 --- a/public/app/features/explore-map/crdt/state.ts +++ b/public/app/features/explore-map/crdt/state.ts @@ -501,10 +501,14 @@ export class CRDTStateManager { const xUpdated = panelData.positionX.set(x, operation.timestamp); const yUpdated = panelData.positionY.set(y, operation.timestamp); + const updated = xUpdated || yUpdated; + + // Note: We don't increment remoteVersion for position changes + // Position updates should not trigger content re-renders return { success: true, - applied: xUpdated || yUpdated, + applied: updated, }; } @@ -518,10 +522,14 @@ export class CRDTStateManager { const widthUpdated = panelData.width.set(width, operation.timestamp); const heightUpdated = panelData.height.set(height, operation.timestamp); + const updated = widthUpdated || heightUpdated; + + // Note: Size changes are handled by width/height props in ExploreMapPanelContent + // They trigger re-renders through the React.memo comparison, not remoteVersion return { success: true, - applied: widthUpdated || heightUpdated, + applied: updated, }; } @@ -535,6 +543,9 @@ export class CRDTStateManager { const updated = panelData.zIndex.set(zIndex, operation.timestamp); + // Note: We don't increment remoteVersion for zIndex changes + // zIndex is a visual property that doesn't affect content + return { success: true, applied: updated, @@ -572,6 +583,11 @@ export class CRDTStateManager { const updated = panelData.iframeUrl.set(iframeUrl, operation.timestamp); + // Increment remoteVersion only for remote operations + if (updated && operation.nodeId !== this.nodeId) { + panelData.remoteVersion++; + } + return { success: true, applied: updated, diff --git a/public/app/features/explore-map/hooks/usePanelStateSync.ts b/public/app/features/explore-map/hooks/usePanelStateSync.ts index 5fe597f2200..019ac2972bd 100644 --- a/public/app/features/explore-map/hooks/usePanelStateSync.ts +++ b/public/app/features/explore-map/hooks/usePanelStateSync.ts @@ -6,6 +6,7 @@ */ import { useEffect, useRef } from 'react'; +import { shallowEqual } from 'react-redux'; import { useDispatch, useSelector } from 'app/types/store'; @@ -30,16 +31,20 @@ export function usePanelStateSync({ panelId, exploreId }: UsePanelStateSyncOptio // Track the last synced state const lastSyncedStateRef = useRef(null); - // Get current selection state - const selectedPanelIds = useSelector((state) => selectSelectedPanelIds(state.exploreMapCRDT)); - const isSelected = selectedPanelIds.includes(panelId); + // Get current selection state - memoized to only change when THIS panel's selection changes + const isSelected = useSelector((state) => { + const selectedIds = selectSelectedPanelIds(state.exploreMapCRDT); + return selectedIds.includes(panelId); + }); - // Get current explore state - const explorePane = useSelector((state) => state.explore?.panes?.[exploreId]); + // Get current explore state - using shallowEqual to prevent re-renders when content is the same + const explorePane = useSelector((state) => state.explore?.panes?.[exploreId], shallowEqual); - // Get the panel's saved state - const panels = useSelector((state) => selectPanels(state.exploreMapCRDT)); - const panel = panels[panelId]; + // Get the panel's saved exploreState only - using shallowEqual to prevent re-renders + const panelExploreState = useSelector((state) => { + const panels = selectPanels(state.exploreMapCRDT); + return panels[panelId]?.exploreState; + }, shallowEqual); useEffect((): void | (() => void) => { // Update tracking ref @@ -48,13 +53,10 @@ export function usePanelStateSync({ panelId, exploreId }: UsePanelStateSyncOptio // Detect deselection (was selected, now not selected) if (previouslySelected && !isSelected) { - console.log('[PanelStateSync] Panel deselected:', panelId); - // Add a small delay to ensure explore state updates have been committed to Redux const syncTimer = setTimeout(() => { // Get current explore state (after delay to ensure it's updated) if (!explorePane) { - console.log('[PanelStateSync] No explore pane found for', exploreId); return; } @@ -68,13 +70,6 @@ export function usePanelStateSync({ panelId, exploreId }: UsePanelStateSyncOptio compact: explorePane.compact, }; - // Log detailed info about queries and datasource - console.log('[PanelStateSync] Current state:', { - datasourceUid: currentState.datasourceUid, - queriesCount: currentState.queries.length, - queries: currentState.queries, - }); - // Create a stable string representation focusing on the important fields const currentStateStr = JSON.stringify({ datasourceUid: currentState.datasourceUid, @@ -82,15 +77,10 @@ export function usePanelStateSync({ panelId, exploreId }: UsePanelStateSyncOptio range: currentState.range, }); - console.log('[PanelStateSync] Last synced state:', lastSyncedStateRef.current?.substring(0, 100)); - console.log('[PanelStateSync] Current state str:', currentStateStr.substring(0, 100)); - // Check if state has changed since last sync const hasChanged = lastSyncedStateRef.current !== currentStateStr; if (hasChanged) { - console.log('[PanelStateSync] State changed, syncing full state'); - // Dispatch sync action with full state dispatch(savePanelExploreState({ panelId, @@ -99,8 +89,6 @@ export function usePanelStateSync({ panelId, exploreId }: UsePanelStateSyncOptio // Update last synced state lastSyncedStateRef.current = currentStateStr; - } else { - console.log('[PanelStateSync] State unchanged, skipping sync'); } }, 100); // Small delay to ensure state is updated @@ -111,15 +99,14 @@ export function usePanelStateSync({ panelId, exploreId }: UsePanelStateSyncOptio // Initialize last synced state from panel's saved state useEffect(() => { - if (panel?.exploreState && lastSyncedStateRef.current === null) { + if (panelExploreState && lastSyncedStateRef.current === null) { // Use the same format as in the main effect for consistency const initStateStr = JSON.stringify({ - datasourceUid: panel.exploreState.datasourceUid, - queries: panel.exploreState.queries || [], - range: panel.exploreState.range, + datasourceUid: panelExploreState.datasourceUid, + queries: panelExploreState.queries || [], + range: panelExploreState.range, }); lastSyncedStateRef.current = initStateStr; - console.log('[PanelStateSync] Initialized with saved state:', initStateStr.substring(0, 100)); } - }, [panel?.exploreState]); + }, [panelExploreState]); } diff --git a/public/app/features/explore-map/operations/validators.ts b/public/app/features/explore-map/operations/validators.ts index 56de9d98a98..150be567fae 100644 --- a/public/app/features/explore-map/operations/validators.ts +++ b/public/app/features/explore-map/operations/validators.ts @@ -83,7 +83,7 @@ export function validateOperation( validateAddComment(operation, opts, errors); break; case 'remove-comment': - validateRemoveComment(operation, errors); + validateRemoveComment(operation, opts, errors); break; case 'batch': validateBatchOperation(operation, opts, errors); diff --git a/public/app/features/explore-map/state/crdtSlice.ts b/public/app/features/explore-map/state/crdtSlice.ts index abc1284d5c3..fe7f4767b17 100644 --- a/public/app/features/explore-map/state/crdtSlice.ts +++ b/public/app/features/explore-map/state/crdtSlice.ts @@ -11,7 +11,7 @@ import { v4 as uuidv4 } from 'uuid'; import { generateExploreId } from 'app/core/utils/explore'; import { CRDTStateManager } from '../crdt/state'; -import { CRDTOperation, CommentData } from '../crdt/types'; +import { CRDTOperation, CommentData, CRDTExploreMapStateJSON } from '../crdt/types'; import { CanvasViewport, SerializedExploreState, UserCursor } from './types'; @@ -41,6 +41,11 @@ export interface ExploreMapCRDTState { cursors: Record; isOnline: boolean; isSyncing: boolean; + activeDrag?: { + draggedPanelId: string; + deltaX: number; + deltaY: number; + }; }; } @@ -70,6 +75,7 @@ export function createInitialCRDTState(mapUid?: string): ExploreMapCRDTState { cursors: {}, isOnline: false, isSyncing: false, + activeDrag: undefined, }, }; } @@ -111,7 +117,7 @@ const crdtSlice = createSlice({ /** * Load CRDT state from server */ - loadState: (state, action: PayloadAction<{ crdtState: any }>) => { + loadState: (state, action: PayloadAction<{ crdtState: CRDTExploreMapStateJSON }>) => { const manager = CRDTStateManager.fromJSON(action.payload.crdtState, state.nodeId); saveCRDTManager(state, manager); // Restore uid from the loaded CRDT state @@ -507,7 +513,7 @@ const crdtSlice = createSlice({ width: sourcePanel.position.width, height: sourcePanel.position.height, }, - (sourcePanel.mode || 'explore') as 'explore' | 'traces-drilldown' | 'metrics-drilldown' | 'profiles-drilldown' | 'logs-drilldown' + sourcePanel.mode || 'explore' ); manager.applyOperation(addOperation); @@ -612,6 +618,20 @@ const crdtSlice = createSlice({ state.local.isSyncing = action.payload.isSyncing; }, + /** + * Set active drag info (for multi-panel drag visual feedback) + */ + setActiveDrag: (state, action: PayloadAction<{ draggedPanelId: string; deltaX: number; deltaY: number }>) => { + state.local.activeDrag = action.payload; + }, + + /** + * Clear active drag info + */ + clearActiveDrag: (state) => { + state.local.activeDrag = undefined; + }, + /** * Clear all state (reset) */ @@ -648,6 +668,8 @@ export const { removeCursor, setOnlineStatus, setSyncingStatus, + setActiveDrag, + clearActiveDrag, clearMap, } = crdtSlice.actions; diff --git a/public/app/features/explore-map/state/selectors.ts b/public/app/features/explore-map/state/selectors.ts index f91d8a49170..629458262d2 100644 --- a/public/app/features/explore-map/state/selectors.ts +++ b/public/app/features/explore-map/state/selectors.ts @@ -24,8 +24,18 @@ function getCRDTManager(state: ExploreMapCRDTState): CRDTStateManager { return CRDTStateManager.fromJSON(json, state.nodeId); } +/** + * Cache for individual panel objects + * Key: panelId, Value: { panel object, serialized panel data for comparison } + */ +const panelCache = new Map(); + /** * Select all panels as a Record (for compatibility with existing UI) + * + * OPTIMIZATION: This selector caches individual panel objects and only creates + * new panel objects when the underlying CRDT data for that specific panel changes. + * This prevents unnecessary re-renders of unchanged panels when another panel is modified. */ export const selectPanels = createSelector( [ @@ -50,11 +60,34 @@ export const selectPanels = createSelector( }; const manager = getCRDTManager(state); const panels: Record = {}; + const currentPanelIds = new Set(manager.getPanelIds()); - for (const panelId of manager.getPanelIds()) { + // Clean up cache for removed panels + for (const cachedPanelId of panelCache.keys()) { + if (!currentPanelIds.has(cachedPanelId)) { + panelCache.delete(cachedPanelId); + } + } + + // Build panels object, reusing cached objects when data hasn't changed + for (const panelId of currentPanelIds) { const panelData = manager.getPanelForUI(panelId); - if (panelData) { - panels[panelId] = panelData as ExploreMapPanel; + if (!panelData) { + continue; + } + + // Serialize the panel data to detect changes + const serializedData = JSON.stringify(panelData); + const cached = panelCache.get(panelId); + + // Reuse cached panel if data hasn't changed + if (cached && cached.data === serializedData) { + panels[panelId] = cached.panel; + } else { + // Panel is new or changed, create new object and cache it + const panel = panelData as ExploreMapPanel; + panels[panelId] = panel; + panelCache.set(panelId, { panel, data: serializedData }); } }