fix(ourlogs): stop drag-to-zoom writing multiple history entries - #119263
Conversation
github-actions
Bot
added
the
Scope: Frontend
| if (a.pathname !== b.pathname) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Explanation] I don't think this is actually hittable still, so I didn't preserve it in navigateIfQueryChanged. Someone please let me know if I'm wrong!
There was a problem hiding this comment.
I think this may have just been an early cheap check rather than the more expensive json stringify'ing one 🤔
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 989368d. Configure here.
Dragging to zoom on the logs chart pushed several browser-history entries per drag, so hitting back once did not return to the previous query. A datetime selection change trips the auto-refresh toggle's "disable on selection change" effect, which calls setAutorefresh. That callback navigated unconditionally and is recreated on every location change, so it re-fired in a loop, each pass pushing a redundant same-URL entry. Only navigate when the query actually changes. Extract that guard into a shared navigateIfQueryChanged util and route the other existing copies (chart zoom, page filters, spans query params) through it too. Fixes LOGS-383 Co-Authored-By: Claude <noreply@anthropic.com>
JoshuaKGoldberg
changed the title
989368d to
1c92e0e
Compare
| if (a.pathname !== b.pathname) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think this may have just been an early cheap check rather than the more expensive json stringify'ing one 🤔
gggritso
left a comment
gggritso
left a comment
There was a problem hiding this comment.
👍🏻 makes sense, thanks!
It's interesting, I feel like there are very few times we actually want a navigation to the same URL to re-render the page, I'm wondering if we should set that as the default at the useNavigate level? Just food for thoughts, maybe people in the FE channel will have opinions!
|
I like that! Posted on Slack: https://sentry.slack.com/archives/C8V02RHC7/p1783611498666769 |
3 participants
LogsAutoRefreshContextwas missing the "onlynavigate()if the query changed" logic from other places likechartZoom. I added it there, then added a sharednavigateIfQueryChangedhelper to deduplicate the half-dozen places it was used.Closes LOGS-383.