remove selectedMarker from view state, instead keep it in url state only

Author: fatadel

src/actions/receive-profile.ts

@@ -41,7 +41,6 @@ import {
 import {
   getSelectedTab,
   getTabFilter,
-  getSelectedMarkers,
 } from 'firefox-profiler/selectors/url-state';
 import {
   getTabToThreadIndexesMap,
@@ -359,8 +358,6 @@ export function finalizeFullProfileView(
       }
     }
 
-    const selectedMarkers = hasUrlInfo ? getSelectedMarkers(getState()) : {};
-
     withHistoryReplaceStateSync(() => {
       dispatch({
         type: 'VIEW_FULL_PROFILE',
@@ -371,7 +368,6 @@ export function finalizeFullProfileView(
         localTracksByPid,
         localTrackOrderByPid,
         timelineType,
-        selectedMarkers,
         ...hiddenTracks,
       });
     });

src/reducers/profile-view.ts

@@ -137,7 +137,6 @@ export const defaultThreadViewOptions: ThreadViewOptions = {
   selectedInvertedCallNodePath: [],
   expandedNonInvertedCallNodePaths: new PathSet(),
   expandedInvertedCallNodePaths: new PathSet(),
-  selectedMarker: null,
   selectedNetworkMarker: null,
   lastSeenTransformCount: 0,
 };
@@ -174,20 +173,6 @@ const viewOptionsPerThread: Reducer<ThreadViewOptionsPerThreads> = (
     case 'PROFILE_LOADED':
       // The view options are lazily initialized. Reset to the default values.
       return {};
-    case 'VIEW_FULL_PROFILE': {
-      // Initialize selectedMarker for each thread from the URL state.
-      const { selectedMarkers } = action;
-      const newState: ThreadViewOptionsPerThreads = {};
-      for (const [threadsKey, markerIndex] of Object.entries(selectedMarkers)) {
-        if (markerIndex !== null) {
-          newState[threadsKey] = {
-            ..._getThreadViewOptions(state, threadsKey),
-            selectedMarker: markerIndex,
-          };
-        }
-      }
-      return Object.keys(newState).length > 0 ? newState : state;
-    }
     case 'BULK_SYMBOLICATION': {
       const { oldFuncToNewFuncsMaps } = action;
       // For each thread, apply oldFuncToNewFuncsMap to that thread's
@@ -342,10 +327,6 @@ const viewOptionsPerThread: Reducer<ThreadViewOptionsPerThreads> = (
           : { expandedNonInvertedCallNodePaths: expandedCallNodePaths }
       );
     }
-    case 'CHANGE_SELECTED_MARKER': {
-      const { threadsKey, selectedMarker } = action;
-      return _updateThreadViewOptions(state, threadsKey, { selectedMarker });
-    }
     case 'CHANGE_SELECTED_NETWORK_MARKER': {
       const { threadsKey, selectedNetworkMarker } = action;
       return _updateThreadViewOptions(state, threadsKey, {
@@ -417,44 +398,24 @@ const viewOptionsPerThread: Reducer<ThreadViewOptionsPerThreads> = (
       });
     }
     case 'UPDATE_URL_STATE': {
-      // When the URL state changes (e.g., via browser back button):
-      // 1. Check if the transform stack has been popped for each thread.
-      //   If so, reset the stored paths, because they may reference call nodes
-      //   that only exist in a transformed tree.
-      //   See: https://github.com/firefox-devtools/profiler/issues/5689.
-      // 2. Sync selected marker with URL state.
-
+      // When the URL state changes (e.g., via browser back button), check if the
+      // transform stack has been popped for each thread. If so, reset the stored paths
+      // because they may reference call nodes that only exist in a transformed tree.
+      // See: https://github.com/firefox-devtools/profiler/issues/5689
       if (!action.newUrlState) {
         return state;
       }
 
-      const { transforms, selectedMarkers } =
-        action.newUrlState.profileSpecific;
+      const { transforms } = action.newUrlState.profileSpecific;
       return objectMap(state, (viewOptions, threadsKey) => {
         const transformStack = transforms[threadsKey] || [];
         const newTransformCount = transformStack.length;
         const oldTransformCount = viewOptions.lastSeenTransformCount;
 
-        // Get the selected marker from URL state for this thread
-        const urlSelectedMarker = selectedMarkers[threadsKey] ?? null;
-        const currentSelectedMarker = viewOptions.selectedMarker;
-
-        // Check if we need to update anything
-        const transformCountChanged = newTransformCount < oldTransformCount;
-        const markerChanged = urlSelectedMarker !== currentSelectedMarker;
-
-        if (!transformCountChanged && !markerChanged) {
-          // No change needed
-          return viewOptions;
-        }
-
-        // Build the updated view options
-        let updatedOptions = { ...viewOptions };
-
-        // If transform count changed, reset the paths
-        if (transformCountChanged) {
-          updatedOptions = {
-            ...updatedOptions,
+        // If transform count changed, reset the paths.
+        if (newTransformCount < oldTransformCount) {
+          return {
+            ...viewOptions,
             selectedNonInvertedCallNodePath: [],
             selectedInvertedCallNodePath: [],
             expandedNonInvertedCallNodePaths: new PathSet(),
@@ -463,15 +424,8 @@ const viewOptionsPerThread: Reducer<ThreadViewOptionsPerThreads> = (
           };
         }
 
-        // If marker changed, sync it from URL state
-        if (markerChanged) {
-          updatedOptions = {
-            ...updatedOptions,
-            selectedMarker: urlSelectedMarker,
-          };
-        }
-
-        return updatedOptions;
+        // No change needed.
+        return viewOptions;
       });
     }
     case 'CHANGE_IMPLEMENTATION_FILTER': {

src/selectors/per-thread/markers.ts

@@ -552,7 +552,7 @@ export function getMarkerSelectorsPerThread(
    * This returns the marker index for the currently selected marker.
    */
   const getSelectedMarkerIndex: Selector<MarkerIndex | null> = (state) =>
-    threadSelectors.getViewOptions(state).selectedMarker;
+    UrlState.getSelectedMarker(state, threadsKey);
 
   /**
    * From the previous value, this returns the full marker object for the

src/selectors/url-state.ts

@@ -31,7 +31,7 @@ import type {
   NativeSymbolInfo,
   TabID,
   IndexIntoSourceTable,
-  SelectedMarkersPerThread,
+  MarkerIndex,
 } from 'firefox-profiler/types';
 
 import type { TabSlug } from '../app-logic/tabs-handling';
@@ -168,8 +168,6 @@ export const getTabFilter: Selector<TabID | null> = (state) =>
   getProfileSpecificState(state).tabFilter;
 export const hasTabFilter: Selector<boolean> = (state) =>
   getTabFilter(state) !== null;
-export const getSelectedMarkers: Selector<SelectedMarkersPerThread> = (state) =>
-  getProfileSpecificState(state).selectedMarkers;
 
 /**
  * This selector does a simple lookup in the set of hidden tracks for a PID, and ensures
@@ -239,6 +237,12 @@ export const getTransformStack: DangerousSelectorWithArguments<
   );
 };
 
+export const getSelectedMarker: DangerousSelectorWithArguments<
+  MarkerIndex | null,
+  ThreadsKey
+> = (state, threadsKey) =>
+  getProfileSpecificState(state).selectedMarkers[threadsKey] ?? null;
+
 export const getIsBottomBoxOpen: Selector<boolean> = (state) => {
   const tab = getSelectedTab(state);
   return getProfileSpecificState(state).isBottomBoxOpenPerPanel[tab];

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

@@ -4428,7 +4428,6 @@ Object {
   },
   "lastSeenTransformCount": 1,
   "selectedInvertedCallNodePath": Array [],
-  "selectedMarker": 1,
   "selectedNetworkMarker": null,
   "selectedNonInvertedCallNodePath": Array [
     0,

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

@@ -1046,11 +1046,11 @@ describe('actions/ProfileView', function () {
       const { dispatch, getState } = storeWithProfile(profile);
 
       expect(
-        selectedThreadSelectors.getViewOptions(getState()).selectedMarker
+        selectedThreadSelectors.getSelectedMarkerIndex(getState())
       ).toEqual(null);
       dispatch(ProfileView.changeSelectedMarker(0, 0));
       expect(
-        selectedThreadSelectors.getViewOptions(getState()).selectedMarker
+        selectedThreadSelectors.getSelectedMarkerIndex(getState())
       ).toEqual(0);
     });
   });

src/test/url-handling.test.ts

@@ -2036,9 +2036,9 @@ describe('selectedMarker', function () {
       profile
     );
 
-    expect(
-      selectedThreadSelectors.getViewOptions(getState()).selectedMarker
-    ).toEqual(markerIndex);
+    expect(selectedThreadSelectors.getSelectedMarkerIndex(getState())).toEqual(
+      markerIndex
+    );
   });
 
   it('handles marker index 0 correctly on marker-chart tab', () => {
@@ -2121,6 +2121,11 @@ describe('selectedMarker', function () {
   });
 
   it('survives a URL round-trip on marker-chart tab', () => {
+    const profile = getProfileWithMarkers([
+      ['Marker A', 0, null],
+      ['Marker B', 1, null],
+      ['Marker C', 2, null],
+    ]);
     const { dispatch, getState } = setup(
       '?thread=0',
       '/public/1ecd7a421948995171a4bb483b7bcc8e1868cc57/marker-chart/'
@@ -2137,13 +2142,16 @@ describe('selectedMarker', function () {
     // URL should contain the marker
     expect(url).toContain(`marker=${markerIndex}`);
 
-    // Parse the URL back
-    const newUrlState = stateFromLocation(
-      new URL(url, 'https://profiler.firefox.com')
+    // Load a fresh store from the URL and verify via selector
+    const { getState: getState2 } = _getStoreWithURL(
+      {
+        pathname:
+          '/public/1ecd7a421948995171a4bb483b7bcc8e1868cc57/marker-chart/',
+        search: new URL(url, 'https://profiler.firefox.com').search,
+      },
+      profile
     );
-
-    // The marker should be in the parsed URL state
-    expect(newUrlState.profileSpecific.selectedMarkers[threadsKey]).toEqual(
+    expect(selectedThreadSelectors.getSelectedMarkerIndex(getState2())).toEqual(
       markerIndex
     );
   });
@@ -2256,9 +2264,9 @@ describe('selectedMarker', function () {
 
     // Select marker 1
     dispatch(changeSelectedMarker(threadsKey, 1));
-    expect(
-      selectedThreadSelectors.getViewOptions(getState()).selectedMarker
-    ).toEqual(1);
+    expect(selectedThreadSelectors.getSelectedMarkerIndex(getState())).toEqual(
+      1
+    );
 
     // Simulate browser navigation by dispatching UPDATE_URL_STATE
     // with a different selected marker (as if we navigated back)
@@ -2274,10 +2282,9 @@ describe('selectedMarker', function () {
     };
     dispatch({ type: 'UPDATE_URL_STATE', newUrlState });
 
-    // View state should now sync with the URL state
-    expect(
-      selectedThreadSelectors.getViewOptions(getState()).selectedMarker
-    ).toEqual(2);
+    expect(selectedThreadSelectors.getSelectedMarkerIndex(getState())).toEqual(
+      2
+    );
 
     // Now simulate navigating to a state with no selected marker
     const urlStateWithNoMarker = {
@@ -2291,9 +2298,8 @@ describe('selectedMarker', function () {
     };
     dispatch({ type: 'UPDATE_URL_STATE', newUrlState: urlStateWithNoMarker });
 
-    // View state should clear the selected marker
-    expect(
-      selectedThreadSelectors.getViewOptions(getState()).selectedMarker
-    ).toEqual(null);
+    expect(selectedThreadSelectors.getSelectedMarkerIndex(getState())).toEqual(
+      null
+    );
   });
 });

src/types/actions.ts

@@ -39,7 +39,6 @@ import type {
   ApiQueryError,
   TableViewOptions,
   DecodedInstruction,
-  SelectedMarkersPerThread,
 } from './state';
 import type { CssPixels, StartEndRange, Milliseconds } from './units';
 import type { BrowserConnectionStatus } from '../app-logic/browser-connection';
@@ -399,7 +398,6 @@ type ReceiveProfileAction =
       readonly localTrackOrderByPid: Map<Pid, TrackIndex[]>;
       readonly timelineType: TimelineType | null;
       readonly selectedTab: TabSlug;
-      readonly selectedMarkers: SelectedMarkersPerThread;
     }
   | {
       readonly type: 'DATA_RELOAD';

src/types/state.ts

@@ -56,7 +56,6 @@ export type ThreadViewOptions = {
   readonly selectedInvertedCallNodePath: CallNodePath;
   readonly expandedNonInvertedCallNodePaths: PathSet;
   readonly expandedInvertedCallNodePaths: PathSet;
-  readonly selectedMarker: MarkerIndex | null;
   readonly selectedNetworkMarker: MarkerIndex | null;
   // Track the number of transforms to detect when they change via browser
   // navigation. This helps us know when to reset paths that may be invalid