-
Notifications
You must be signed in to change notification settings - Fork 450
Clear selected and expanded call node paths on browser back button if it removes transforms #5701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5701 +/- ##
=======================================
Coverage 85.62% 85.63%
=======================================
Files 312 312
Lines 30880 30892 +12
Branches 8512 8420 -92
=======================================
+ Hits 26441 26453 +12
Misses 4009 4009
Partials 430 430 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mstange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, even if it feels a bit brittle.
I wonder if we have other state outside the UrlState which is affected by actions that also affect the UrlState, and which needs to be kept "in sync" with those changes. If that were the case, we might need to handle UPDATE_URL_STATE in more places to avoid similar bugs.
Ideally the answer would be "no, our actions affect either only state in the UrlState or only state outside the UrlState". So in the long run we should move these thread view options into the UrlState.
Yeah, I couldn't find more like that, but it's possible. We should still have a proper audit and move them if needed. |
… it removes transforms When using the browser back button after applying a focus-category transform, re-applying the transform would fail with the error: "We couldn't find a node with prefix -1 and func X, this shouldn't happen." The root cause was that expanded call node paths from the transformed tree remained in state when navigating back via the browser. These paths were invalid in the untransformed tree, and they were causing errors when the transform was re-applied. The fix adds a lastSeenTransformCount field to ThreadViewOptions to track the number of transforms applied to each thread. When UPDATE_URL_STATE fires via browser back button, we compare the new transform count with the old count. If we notice that a transform was removed, we reset all call node paths to ensure they're always valid for the current tree structure. This matches the existing behavior of POP_TRANSFORMS_FROM_STACK (removing the transform from the filter navigator bar), which already resets paths correctly.
c375f0f to
27f4f53
Compare
… it removes transforms (firefox-devtools#5701) Fixes firefox-devtools#5689. The root cause of this issue was that expanded call node paths from the transformed tree remained in state when navigating back via the browser. These paths were invalid in the untransformed tree, and they were causing errors when the transform was re-applied. The fix adds a lastSeenTransformCount field to ThreadViewOptions to track the number of transforms applied to each thread. When UPDATE_URL_STATE fires via browser back button, we compare the new transform count with the old count. If we notice that a transform was removed, we reset all call node paths to ensure they're always valid for the current tree structure. This matches the existing behavior of POP_TRANSFORMS_FROM_STACK (removing the transform from the filter navigator bar), which already resets paths.
Changes: [Markus Stange] Use a longer test timeout when debugging with VS code. (#5679) [Markus Stange] Move Jest config from package.json to jest.config.js (#5680) [Markus Stange] Make binary profile format parsing use Uint8Array instead of ArrayBuffer (#5678) [Markus Stange] Use workbox-cli to generate the service worker (#5681) [Nazım Can Altınova] Migrate from Appveyor to GitHub Actions Windows runners (#5660) [Nazım Can Altınova] Remove some unused dependencies (#5696) [Nazım Can Altınova] Update the document links and sections (#5705) [Nazım Can Altınova] Clear selected and expanded call node paths on browser back button if it removes transforms (#5701) [Nazım Can Altınova] Properly type the Map and Set objects (#5623) [Valentin Gosu] Add priorityHeader field to network requests (#5707) [Nazım Can Altınova] Redirect unpublished url loads to the homepage similar to from-file (#5712) [Florian Quèze/Nazım Can Altınova] Add an importer for the text format taken as input by flamegraph.pl. (#5359) [Florian Quèze] Improve the import of profiles generated from clang -ftime-trace=file.json (#5714) [Markus Stange] Move React stuff out of marker schema logic module. (#5720) And thanks to our localizers: en-CA: chutten en-CA: Paul es-CL: ravmn fr: Théo Chevalier fur: Fabio Tomat ru: berry tr: Selim Şumlu zh-CN: Olvcpr423 zh-CN: wxie
Fixes #5689.
The root cause of this issue was that expanded call node paths from the transformed tree remained in state when navigating back via the browser. These paths were invalid in the untransformed tree, and they were causing errors when the transform was re-applied.
The fix adds a lastSeenTransformCount field to ThreadViewOptions to track the number of transforms applied to each thread. When UPDATE_URL_STATE fires via browser back button, we compare the new transform count with the old count. If we notice that a transform was removed, we reset all call node paths to ensure they're always valid for the current tree structure.
This matches the existing behavior of POP_TRANSFORMS_FROM_STACK (removing the transform from the filter navigator bar), which already resets paths.
Deploy preview: before / after