Skip to content

Commit 90fe67e

Browse files
authored
Clear selected and expanded call node paths on browser back button if it removes transforms (#5701)
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.
2 parents af28bc2 + 27f4f53 commit 90fe67e

File tree

5 files changed

+109
-1
lines changed

5 files changed

+109
-1
lines changed

src/profile-logic/transforms.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,7 @@ function _dropFunctionInCallNodePath(
600600
return callNodePath.includes(funcIndex) ? [] : callNodePath;
601601
}
602602

603-
// removes all functions that are not in the category from the callNodePath
603+
// Removes all functions that are not in the category from the callNodePath
604604
function _removeOtherCategoryFunctionsInNodePathWithFunction(
605605
category: IndexIntoCategoryList,
606606
callNodePath: CallNodePath,

src/reducers/profile-view.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ export const defaultThreadViewOptions: ThreadViewOptions = {
139139
expandedInvertedCallNodePaths: new PathSet(),
140140
selectedMarker: null,
141141
selectedNetworkMarker: null,
142+
lastSeenTransformCount: 0,
142143
};
143144

144145
function _getThreadViewOptions(
@@ -378,11 +379,15 @@ const viewOptionsPerThread: Reducer<ThreadViewOptionsPerThreads> = (
378379
threadViewOptions.expandedInvertedCallNodePaths
379380
);
380381

382+
const lastSeenTransformCount =
383+
threadViewOptions.lastSeenTransformCount + 1;
384+
381385
return _updateThreadViewOptions(state, threadsKey, {
382386
selectedNonInvertedCallNodePath,
383387
selectedInvertedCallNodePath,
384388
expandedNonInvertedCallNodePaths,
385389
expandedInvertedCallNodePaths,
390+
lastSeenTransformCount,
386391
});
387392
}
388393
case 'POP_TRANSFORMS_FROM_STACK': {
@@ -394,6 +399,38 @@ const viewOptionsPerThread: Reducer<ThreadViewOptionsPerThreads> = (
394399
selectedInvertedCallNodePath: [],
395400
expandedNonInvertedCallNodePaths: new PathSet(),
396401
expandedInvertedCallNodePaths: new PathSet(),
402+
lastSeenTransformCount: 0,
403+
});
404+
}
405+
case 'UPDATE_URL_STATE': {
406+
// When the URL state changes (e.g., via browser back button), check if the
407+
// transform stack has been popped for each thread. If so, reset the stored paths
408+
// because they may reference call nodes that only exist in a transformed tree.
409+
// See: https://github.com/firefox-devtools/profiler/issues/5689
410+
if (!action.newUrlState) {
411+
return state;
412+
}
413+
414+
const { transforms } = action.newUrlState.profileSpecific;
415+
return objectMap(state, (viewOptions, threadsKey) => {
416+
const transformStack = transforms[threadsKey] || [];
417+
const newTransformCount = transformStack.length;
418+
const oldTransformCount = viewOptions.lastSeenTransformCount;
419+
420+
// If transform count changed, reset the paths.
421+
if (newTransformCount < oldTransformCount) {
422+
return {
423+
...viewOptions,
424+
selectedNonInvertedCallNodePath: [],
425+
selectedInvertedCallNodePath: [],
426+
expandedNonInvertedCallNodePaths: new PathSet(),
427+
expandedInvertedCallNodePaths: new PathSet(),
428+
lastSeenTransformCount: newTransformCount,
429+
};
430+
}
431+
432+
// No change needed.
433+
return viewOptions;
397434
});
398435
}
399436
case 'CHANGE_IMPLEMENTATION_FILTER': {

src/test/store/__snapshots__/profile-view.test.ts.snap

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4393,6 +4393,7 @@ Object {
43934393
],
43944394
},
43954395
},
4396+
"lastSeenTransformCount": 1,
43964397
"selectedInvertedCallNodePath": Array [],
43974398
"selectedMarker": 1,
43984399
"selectedNetworkMarker": null,

src/test/store/transforms.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import {
2525
changeSelectedCallNode,
2626
changeCallTreeSummaryStrategy,
2727
} from '../../actions/profile-view';
28+
import * as AppActions from '../../actions/app';
29+
import * as UrlStateSelectors from '../../selectors/url-state';
2830
import { selectedThreadSelectors } from '../../selectors/per-thread';
2931

3032
describe('"focus-subtree" transform', function () {
@@ -819,6 +821,70 @@ describe('"focus-category" transform', function () {
819821
expect(selectedCallNodePath).toEqual([A, A]);
820822
});
821823
});
824+
825+
describe('browser back button behavior', function () {
826+
// This test ensures that when using browser back button after applying
827+
// a focus-category transform, re-applying the transform doesn't fail.
828+
// The bug occurred because expanded paths from the transformed tree
829+
// weren't being reset when the URL state changed via browser navigation.
830+
const { threadIndex, categoryIndex, funcNamesDict, getState, dispatch } =
831+
setup(`
832+
A[cat:Other] A[cat:Other]
833+
B[cat:Other] B[cat:Other]
834+
C[cat:Graphics] C[cat:Graphics]
835+
D[cat:Graphics] D[cat:Graphics]
836+
E[cat:Graphics] F[cat:Graphics]
837+
`);
838+
839+
it('can re-apply transform after browser back without error', function () {
840+
const { C, D, E } = funcNamesDict;
841+
842+
// Apply focus-category transform.
843+
dispatch(
844+
addTransformToStack(threadIndex, {
845+
type: 'focus-category',
846+
category: categoryIndex,
847+
})
848+
);
849+
850+
// Select a deep node in the transformed tree, which expands paths.
851+
// In the transformed tree, C becomes a root since A and B are filtered out.
852+
dispatch(changeSelectedCallNode(threadIndex, [C, D, E]));
853+
854+
expect(
855+
selectedThreadSelectors.getSelectedCallNodePath(getState())
856+
).toEqual([C, D, E]);
857+
858+
// Capture the current URL state with transforms.
859+
const urlStateWithTransforms = UrlStateSelectors.getUrlState(getState());
860+
861+
// Simulate browser back button by creating a URL state without transforms.
862+
const urlStateWithoutTransforms = {
863+
...urlStateWithTransforms,
864+
profileSpecific: {
865+
...urlStateWithTransforms.profileSpecific,
866+
transforms: {},
867+
},
868+
};
869+
870+
// Apply the URL state change (simulating browser back).
871+
dispatch(AppActions.updateUrlState(urlStateWithoutTransforms));
872+
873+
// Re-apply the same transform. This should not throw an error.
874+
expect(() => {
875+
dispatch(
876+
addTransformToStack(threadIndex, {
877+
type: 'focus-category',
878+
category: categoryIndex,
879+
})
880+
);
881+
}).not.toThrow();
882+
883+
expect(
884+
selectedThreadSelectors.getSelectedCallNodePath(getState())
885+
).toEqual([]);
886+
});
887+
});
822888
});
823889

824890
describe('"collapse-resource" transform', function () {

src/types/state.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ export type ThreadViewOptions = {
5858
readonly expandedInvertedCallNodePaths: PathSet;
5959
readonly selectedMarker: MarkerIndex | null;
6060
readonly selectedNetworkMarker: MarkerIndex | null;
61+
// Track the number of transforms to detect when they change via browser
62+
// navigation. This helps us know when to reset paths that may be invalid
63+
// in the new transform state.
64+
readonly lastSeenTransformCount: number;
6165
};
6266

6367
export type ThreadViewOptionsPerThreads = {

0 commit comments

Comments
 (0)