feat: get rid of ReduxTooltip #3192
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR removes the centralized Redux-based tooltip system (ReduxTooltip) and replaces it with local state-based tooltip management using Gravity UI's Popup component in individual components. This simplifies the architecture by eliminating global tooltip state and reduces Redux boilerplate.
- Removes Redux tooltip slice, actions, and middleware configuration
- Migrates three tooltip use cases to local implementations: Network node tooltips, Heatmap histogram/tablet tooltips, and QueryResultTable cell tooltips
- Converts Histogram component from JavaScript to TypeScript with proper typing
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/tooltip.js |
Deleted: Removed tooltip template definitions (node, histogram, cell, tablet) |
src/types/store/tooltip.ts |
Deleted: Removed Redux tooltip type definitions |
src/store/reducers/tooltip.ts |
Deleted: Removed Redux tooltip reducer and action creators |
src/store/reducers/index.ts |
Removed tooltip reducer from root reducer configuration |
src/store/configureStore.ts |
Removed tooltip-specific Redux middleware configuration for immutability/serializability checks |
src/containers/ReduxTooltip/ReduxTooltip.scss |
Deleted: Removed centralized tooltip styles |
src/containers/ReduxTooltip/ReduxTooltip.js |
Deleted: Removed Redux-connected tooltip container component |
src/containers/App/App.tsx |
Removed ReduxTooltip component from app root |
src/containers/Tenant/Diagnostics/Network/NodeNetwork/NodeNetwork.tsx |
Added TypeScript types for tooltip data and improved null safety |
src/containers/Tenant/Diagnostics/Network/Network.tsx |
Implemented local tooltip state with Popup component for node tooltips |
src/containers/Heatmap/Histogram/Histogram.tsx |
New: Converted from JS to TS, added local tooltip state management |
src/containers/Heatmap/Histogram/Histogram.js |
Deleted: Replaced by TypeScript version |
src/containers/Heatmap/HeatmapCanvas/HeatmapCanvas.tsx |
Converted from JS to TS with proper typing, improved tooltip positioning logic |
src/containers/Heatmap/Heatmap.tsx |
Implemented local tablet tooltip state with hover persistence logic |
src/components/QueryResultTable/QueryResultTable.tsx |
Added local active cell state management for cell tooltips |
src/components/QueryResultTable/QueryResultTable.scss |
Added cell popup styles (migrated from ReduxTooltip.scss) |
src/components/QueryResultTable/Cell/Cell.tsx |
Implemented inline Popup component for cell tooltips |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (2)
src/containers/Heatmap/HeatmapCanvas/HeatmapCanvas.tsx:83
- Potential division by zero when calculating
rowsCountifcolumnsCountis 0 (which could happen if container width is very small). Add a safety check:
const columnsCount = Math.floor(width / (TABLET_SIZE + TABLET_PADDING));
if (columnsCount === 0) {
return; // or set some default dimensions
}
const rowsCount = Math.ceil(tabletsLength / columnsCount);src/containers/Heatmap/HeatmapCanvas/HeatmapCanvas.tsx:198
- The throttled function
_onCanvasMouseMoveis recreated on every render. Wrap it withuseMemooruseCallbackto maintain the same throttled instance across renders:
const _onCanvasMouseMove = React.useMemo(
() => throttle((x: number, y: number) => {
const parent = props.parentRef.current;
if (!parent) {
return;
}
const xPos = x - getOffsetLeft() + parent.scrollLeft;
const yPos = y - getOffsetTop() + parent.scrollTop;
const tabletIndex = getTabletIndex(xPos, yPos);
const tablet = tablets[tabletIndex];
if (tablet) {
const {columnsCount} = dimensions;
const colIndex = tabletIndex % columnsCount;
const rowIndex = Math.floor(tabletIndex / columnsCount);
const rectX = colIndex * (TABLET_SIZE + TABLET_PADDING);
const rectY = rowIndex * (TABLET_SIZE + TABLET_PADDING);
const left = getOffsetLeft() - parent.scrollLeft + rectX + TABLET_SIZE / 2;
const top = getOffsetTop() - parent.scrollTop + rectY + TABLET_SIZE / 2;
onShowTabletTooltip(tablet, {left, top});
} else {
onHideTabletTooltip();
}
}, 20),
[tablets, dimensions, onShowTabletTooltip, onHideTabletTooltip, props.parentRef]
);Also consider cleanup: return a cleanup function from useEffect to cancel the throttled function on unmount.
src/containers/Tenant/Diagnostics/Network/NodeNetwork/NodeNetwork.tsx
Outdated
Show resolved
Hide resolved
astandrik
requested review from
Raubzeug,
adameat and
artemmufazalov
as code owners
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }; | ||
| }; | ||
|
|
||
| function activeCellReducer(state: ActiveCellState, action: ActiveCellAction): ActiveCellState { |
There was a problem hiding this comment.
lets delegate this logic to Cell components. It will simplify structure a lot.
| cursor: pointer; | ||
| } | ||
|
|
||
| &__tooltip-anchor { |
src/containers/Heatmap/Heatmap.tsx
Outdated
|
|
||
| return null; | ||
| }); | ||
| }, [isTabletTooltipHovered]); |
There was a problem hiding this comment.
Bug: Stale closure causes tooltip to hide when hovered
The handleRequestHideTabletTooltip callback captures isTabletTooltipHovered from its closure, but when called from the setTimeout in HeatmapCanvas._onCanvasMouseLeave, it uses a stale value. When the user moves their mouse from the canvas to the tooltip, the setTimeout fires with the old function reference that has isTabletTooltipHovered = false captured, causing the tooltip to hide even though the user is hovering over it. The isTabletTooltipHovered check is effectively bypassed because the timeout callback was created before the hover state changed.
Additional Locations (1)
astandrik
force-pushed
the
astandrik.2342-3
branch
from
3242259 to
25363d4
Compare
Closes #2342
Closes #2341
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 62.52 MB | Main: 62.50 MB
Diff: +0.02 MB (0.03%)
ℹ️ CI Information
Improvements
Minor Issues
Network.tsxuses hardcoded class names'error'and'loader'instead of BEM convention (addressed in style comments)Confidence Score: 4/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant User participant Component participant Popup participant State Note over User,State: Old Flow (Redux-based) User->>Component: Hover/Click Component->>Redux: dispatch(showTooltip()) Redux->>ReduxTooltip: Update global state ReduxTooltip->>Popup: Render tooltip User->>Component: Move away Component->>Redux: dispatch(hideTooltip()) Redux->>ReduxTooltip: Clear state ReduxTooltip->>Popup: Hide tooltip Note over User,State: New Flow (Local state) User->>Component: Hover/Click on Cell/Node/Histogram Component->>State: setState with tooltip data State->>Popup: Render Gravity UI Popup Popup->>User: Display tooltip User->>Popup: Click outside or leave Popup->>State: Clear tooltip state State->>Popup: Hide tooltipContext used:
dashboard- Use the BEM naming convention with theb()utility function for CSS classes instead of hardcoded c... (source)dashboard- Remove unused interfaces and CSS classes that are added during development. Clean up duplicate code ... (source)Note
Remove global Redux tooltip and migrate tooltips to local Gravity UI Popups across components with TypeScript and i18n updates.
configureStore.ts); dropReduxTooltipfromApp.Cell: add click-to-togglePopupshowing cell value; add__cell-popupstyles.Popupfor tablet tooltips with anchor positioning; add hover handling.HeatmapCanvasto TS; draw tablets and report tooltip positions via callbacks.Histogramwith TS version using localPopupand i18n; add tooltip styles.NodeTooltipPopup; manage tooltip state locally.Nodes,NetworkPlaceholder; add i18n and styles; movegroupNodesByFieldtoutils.QueryResultTable.tsx; SCSS additions for tooltips.Written by Cursor Bugbot for commit 25363d4. This will update automatically on new commits. Configure here.
Greptile Overview
Greptile Summary
This PR successfully removes the Redux-based global tooltip system and migrates to component-local state management using Gravity UI's
Popupcomponent. The refactoring improves code maintainability by eliminating global state for UI concerns and provides better encapsulation.Key Changes:
ReduxTooltipcomponent, tooltip reducer, actions, and type definitionsCell,Heatmap,HeatmapCanvas,Histogram, andNetworkcomponents to use localPopupstateHeatmapCanvasandHistogramfrom JavaScript to TypeScript with proper typingReduxTooltip.scssto component-specific stylesheetsIssues Found:
Network.tsxuses hardcoded class names'loader'and'error'instead of BEM convention withb()utility (lines 92, 99)Confidence Score: 4/5
Network.tsx) that don't affect functionality. Tests show only 1 flaky test unrelated to tooltip changes.Important Files Changed
File Analysis
'loader'and'error'instead of BEM convention on lines 92 and 99Sequence Diagram
sequenceDiagram participant User participant Component participant LocalState participant Popup participant Redux Note over User,Redux: Old Flow (Redux-based) User->>Component: Hover/Click Element Component->>Redux: dispatch(showTooltip(data)) Redux->>Redux: Update global tooltip state Redux->>ReduxTooltip: Render with global state ReduxTooltip->>Popup: Render Gravity UI Popup User->>Component: Move away Component->>Redux: dispatch(hideTooltip()) Redux->>ReduxTooltip: Clear state ReduxTooltip->>Popup: Hide tooltip Note over User,Redux: New Flow (Local state) User->>Component: Hover/Click on Cell/Node/Histogram Component->>LocalState: setState({anchor, data}) LocalState->>Popup: Render Gravity UI Popup Popup->>User: Display tooltip User->>Popup: Click outside or leave Popup->>Component: onOutsideClick/onMouseLeave Component->>LocalState: Clear tooltip state LocalState->>Popup: Hide tooltipContext used:
dashboard- Use the BEM naming convention with theb()utility function for CSS classes instead of hardcoded c... (source)