feat(explore): Add to filter option in heatmaps tooltip#116690
Conversation
github-actions
Bot
added
the
Scope: Frontend
📊 Type Coverage Diff
🔍 3 new type safety issues introducedType assertions (
This is informational only and does not block the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const localQueryUpdateTarget = (e.target as Element).closest( | ||
| '[data-local-query-update-url]' | ||
| ); | ||
| const tracesLinkTarget = (e.target as Element).closest('[data-traces-link]'); |
There was a problem hiding this comment.
Text clicks break tooltip routing
Medium Severity
The document click handler calls closest on event.target cast to Element, but clicks on tooltip link labels often target a Text node, which has no closest. That throws before preventDefault, so client-side navigate is skipped and the browser follows href with a full page reload.
Reviewed by Cursor Bugbot for commit b81dc9d. Configure here.
There was a problem hiding this comment.
idk what they're talking about the client side navigation works
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aa52596. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // yes i am aware that this is UGLY but it's a hack so that we can use proper react routing. | ||
| // Basically the way ECharts renders the tooltip is by creating a string out of the dom tree. | ||
| // This means that we can't use any of the normal linking/routing tools that we use in React trees | ||
| // because they require contexts that won't be available properly in this string tree. | ||
| // Using the `<a>` tag will make the page reload and navigate to the url because it doesn't have | ||
| // link history context. Doing the navigation here preserves the link history context and makes the | ||
| // page navigation smoother instead of reloading the page every time a link is clicked. |
| [metric, organization] | ||
| ); | ||
|
|
||
| const getUpdatedMetricsQueryUrl = useCallback( |
There was a problem hiding this comment.
nit: Since we're setting the query here, we may want to change the name
| scale?: 'linear' | 'log'; | ||
| /** | ||
| * getExploreUrl props that will be used to generate an explore link for the tooltip. Omitting this will not generate an explore link. | ||
| * Callback that returns an updated query string. |
There was a problem hiding this comment.
nit: Since we're setting this directly and not returning anything in it, should the description be updated 👀
2 participants
few things being done here. Main thing is we've added another optional function to pass in to the
HeatMapWidgetVisualization; theupdateLocalFilterQueryfunction is a callback that updates the local url using the query that we pass it. So far it's only being used for the metrics product so we've passed in a metrics query updater.Other things that were done in this PR
navigatein a useEffect instead of the<a>tag. Without this the entire page reloads and navigation between filtered pages is not smooth (unfortunately the code is a little ugly for this 😭 but if anyone knows a better way to do this plz lemme know)Screen.Recording.2026-06-03.at.11.31.09.AM.mov
Closes DAIN-1651