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
|
@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)
| <TabletTooltipContent data={tabletTooltip.tablet} /> | ||
| </div> | ||
| </Popup> | ||
| ) : null} |
There was a problem hiding this comment.
Bug: Popup may render before anchor element is ready
The Popup component receives tabletTooltipAnchorElement which is null on the first render cycle when tabletTooltip becomes truthy. The anchor div is created with a callback ref (setTabletTooltipAnchorElement), but this state update only takes effect after the initial render. This means the Popup opens with a null anchor element initially, potentially causing it to appear in the wrong position or not at all until the second render.
Closes #2342
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 62.51 MB | Main: 62.48 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
Replaces global Redux tooltips with component-local Gravity UI Popups in QueryResultTable, Heatmap, Histogram, and Network; removes tooltip store and adds TS/i18n updates.
ReduxTooltipcomponent,store/reducers/tooltip,types/store/tooltip, andutils/tooltip; drop tooltip wiring fromApp.tsx, store config, and root reducers.QueryResultTable:PopupinCell.tsx; add__cell-popupstyles.Heatmap:Popup; add anchor/hover handling.HeatmapCanvasto TypeScript and emit tooltip coordinates via callbacks.Histogramto TypeScript with localPopuptooltip and i18n.Network:NodeTooltipPopupand local state; addNetworkPlaceholderandNodescomponent; extract helpers toutils.ts; add i18n.QueryResultTable.scss,Heatmap.scss,Histogram.scss,Network.scss).Written by Cursor Bugbot for commit 3242259. This will update automatically on new commits. Configure here.
Greptile Overview
Greptile Summary
This PR successfully removes the global Redux-based tooltip system and replaces it with component-local Gravity UI
Popupimplementations. The refactoring improves code organization by giving each component ownership of its tooltip state throughuseStateanduseCallbackhooks.Key Changes:
ReduxTooltipcomponent, tooltip reducer, actions, and utilitiesCell,Heatmap,Histogram, andNetworkHeatmapCanvasandHistogramfrom JavaScript to TypeScript with proper typingReduxTooltip.scssto component-specific stylesheetsImplementation Quality:
Minor Style Issues:
Network.tsx:92-99uses hardcoded class names'loader'and'error'instead of BEM convention withb()utility (violates style guide rule 2bea9cbf-2d5a-4f2d-8669-abfdb73f2fc4)Confidence Score: 4/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant User participant Component participant LocalState participant Popup Note over User,Popup: Old Flow (Redux-based) User->>Component: Hover/Click Component->>Redux: dispatch(showTooltip()) Redux->>ReduxTooltip: Update global state ReduxTooltip->>Popup: Render at coordinates User->>Component: Move away Component->>Redux: dispatch(hideTooltip()) Note over User,Popup: New Flow (Local State) User->>Component: Hover on Cell/Node/Histogram Component->>LocalState: setState({anchor, data}) LocalState->>Popup: Render Gravity UI Popup Popup->>User: Display tooltip User->>Popup: Click outside / Mouse leave Popup->>LocalState: Clear state LocalState->>Component: Hide tooltipContext used:
dashboard- Use the BEM naming convention with theb()utility function for CSS classes instead of hardcoded c... (source)