Persist the selected marker in the url by fatadel · Pull Request #5847 · firefox-devtools/profiler

fatadel · 2026-02-17T08:27:22Z

Closes #5241.

This PR adds a possibility to persist the selected marker in the URL, so that, when sharing profiles, the view of the two users matches as much as possible.

Before

https://profiler.firefox.com/public/707b9xarf5kkjg49pn1rrgs8fg20mrz554wdc1g/marker-chart/?globalTrackOrder=fwi0we&thread=0&v=13

After

https://deploy-preview-5847--perf-html.netlify.app/public/707b9xarf5kkjg49pn1rrgs8fg20mrz554wdc1g/marker-chart/?globalTrackOrder=fwi0we&marker=5173&thread=0&v=14

As you may notice, now the URL has marker= query param that is responsible for persisting the selected marker. The URL version is bumped to version 14.

codecov · 2026-02-17T08:29:44Z

Codecov Report

❌ Patch coverage is 86.20690% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.57%. Comparing base (37b31b0) to head (946a4ab).
⚠️ Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/shared/chart/Canvas.tsx	70.00%	9 Missing ⚠️
src/components/marker-chart/Canvas.tsx	87.03%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5847      +/-   ##
==========================================
+ Coverage   85.56%   85.57%   +0.01%     
==========================================
  Files         319      319              
  Lines       31420    31560     +140     
  Branches     8661     8711      +50     
==========================================
+ Hits        26885    27009     +124     
- Misses       4104     4120      +16     
  Partials      431      431

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

canova

Thanks for the PR!

I would prefer to merge the 2 dispatches inside finalizeFullProfileView, see my comment below.

I noticed that when you scroll down the marker chart and select the marker, it doesn't scroll enough to keep the marker in the viewport. For example:
https://deploy-preview-5847--perf-html.netlify.app/public/707b9xarf5kkjg49pn1rrgs8fg20mrz554wdc1g/marker-chart/?globalTrackOrder=fwi0we&marker=46160&thread=0&v=14
Can you look into that?

I couldn't finish my full review yet (for example getTooltipPosition etc.), but sending my initial findings beforehand.

canova · 2026-02-17T21:01:28Z

src/actions/receive-profile.ts

+    // Initialize selected markers from URL state if present
+    if (hasUrlInfo) {
+      const selectedMarkers = getSelectedMarkers(getState());
+      // Dispatch marker selection for each thread that has a marker in URL
+      for (const [threadsKey, markerIndex] of Object.entries(selectedMarkers)) {
+        if (markerIndex !== null) {
+          dispatch(changeSelectedMarker(threadsKey, markerIndex));
+        }
+      }
+    }
  };


Hmm, is there a reason why this is not merged with VIEW_FULL_PROFILE? Currently, we only have single dispatch inside finalizeFullProfileView and we put all the state update there to update it once. We should ideally keep it that way. We can update the relevant reducers to update the state on VIEW_FULL_PROFILE. How does the transform url param handles this?

Yes, what you say is 100% valid, I kinda missed that logic. I've made a commit now for this specific piece of feedback - could you pls have a quick look if I got you correctly?

(resolved irl)

canova · 2026-02-17T21:18:11Z

src/components/shared/chart/Canvas.tsx

+  override componentDidMount() {
+    // Initialize selectedItem from props on mount if provided
+    // Use requestAnimationFrame to ensure the canvas is fully laid out
+    if (this.props.selectedItem !== undefined) {


Nit: If you assign selectedItem to a const variable, then you won't have to assert below with !.

For example:

const { hoveredItem } = this.props; if (selectedItem !== undefined) { ... this._syncSelectedItemFromProp(selectedItem);

It's more accurate type-wise because typescript now knows that it can't be changed in between componentDidMount and requestAnimationFrame callback.

But apart from that, I don't think window.requestAnimationFrame is great here. Can you tell me why it was needed? Maybe we can find a different way.

Regarding the nit:

It is usually true, yes, but not here, because there are different scopes accessing this.props.selectedItem. One of them attaches the callback, another access is inside the callback itself. So, removing the exclamation mark will yield TS2345: Argument of type 'Item | null | undefined' is not assignable to parameter of type 'Item | null'. Type 'undefined' is not assignable to type 'Item | null'.

Now regarding the usage of window.requestAnimationFrame: to be honest, I did not have it in the first place too, but I've asked claude to review my code before submitting. The feedback from claude was the following:

The requestAnimationFrame is necessary here. Inside _syncSelectedItemFromProp, we call this._canvas.getBoundingClientRect():

const canvasRect = this._canvas.getBoundingClientRect(); const pageX = canvasRect.left + window.scrollX + tooltipPosition.offsetX; const pageY = canvasRect.top + window.scrollY + tooltipPosition.offsetY;

componentDidMount fires right after the component's DOM nodes are inserted into the document, but before the browser has performed layout. At that moment, the canvas exists in the DOM but may not yet have its final size and position calculated — getBoundingClientRect() could return zeros or stale values.

requestAnimationFrame delays the call until just before the next paint, by which point the browser has done a layout pass and getBoundingClientRect() returns accurate values.

Without it, on initial load the tooltip would likely appear at position (0, 0) or some incorrect location because the canvas's bounding rect hasn't been computed yet.

Why this only affects componentDidMount and not componentDidUpdate: by the time componentDidUpdate runs, the component has already been rendered at least once and layout has occurred, so getBoundingClientRect() is already reliable.

It seemed as valid feedback to me. But if you don't agree - let's discuss.

It is usually true, yes, but not here, because there are different scopes accessing this.props.selectedItem. One of them attaches the callback, another access is inside the callback itself. So, removing the exclamation mark will yield TS2345: Argument of type 'Item | null | undefined' is not assignable to parameter of type 'Item | null'. Type 'undefined' is not assignable to type 'Item | null'.

That's why I suggested to assign the selectedItem to another constant variable. (ah now I see that in my example I used hoveredItem instead of selectedItem. So the full code would be:

const { selectedItem } = this.props; if (selectedItem !== undefined) { window.requestAnimationFrame(() => { this._syncSelectedItemFromProp(selectedItem); }); }

And thinking about it, this callback has a bug because of it. First we check this.props.selectedItem to see if it's not undefined. Then we use it inside this callback, what happens if the value of it is changed in between?

For the rAF, there is really no guarantee that the Viewport update will happen in the next rAF callback. So it looks a bit racy to me.

So I would probably:

In componentDidMount: Call _syncSelectedItemFromProp directly, but only if containerWidth !== 0. If it's still 0, skip it, componentDidUpdate will pick it up.

In componentDidUpdate: Add a guard for when prevProps.containerWidth === 0 && this.props.containerWidth !== 0 && this.props.selectedItem !== undefined.

This will remove the rAF and it will make sure that the sync happens after the layout. But I haven't tested it fully. Let me know if you think otherwise!

You're right, I've rewritten this piece of code.

canova · 2026-02-17T21:29:06Z

src/app-logic/url-handling.ts

  }

+  // Parse the selected marker for the current thread
+  const selectedMarkers: { [key: string]: MarkerIndex | null } = {};


Nit: We can use the SelectedMarkersPerThread type here directly.

fatadel · 2026-02-23T15:24:44Z

@canova Thank you very much for the valuable feedback! I think I went thru all the points you've raised and now this PR should be ready for another review :)

… dispatches

…ync on load

canova

Nice, thanks a lot for working on it! It looks pretty good now. I had some comments below.

I like that the tooltip now follows the marker properly when we scroll!

I noticed that the initial scroll is not always working 100%. For example, see this link. I selected the "FirstContentfulComposite" marker and I can't really see the marker in the viewport. I'm not 100% sure why it's happening yet. Could you look into it?

canova · 2026-02-25T11:44:07Z

src/app-logic/url-handling.ts

 import { StringTable } from 'firefox-profiler/utils/string-table';

-export const CURRENT_URL_VERSION = 13;
+export const CURRENT_URL_VERSION = 14;


Thanks for updating the url version! It's good to do it.

canova · 2026-02-25T11:45:06Z

src/app-logic/url-handling.ts

      query = baseQuery as MarkersQueryShape;
      query.markerSearch =
        urlState.profileSpecific.markersSearchString || undefined;
+      query.marker =


Can we maybe add a TODO for supporting network-chart in the future too

canova · 2026-02-25T11:56:00Z

src/app-logic/url-handling.ts

+  ) {
+    const markerIndex = Number(query.marker);
+    if (!isNaN(markerIndex)) {
+      selectedMarkers[selectedThreadsKey] = markerIndex;


I wish there was a way to further validate if the marker index is valid at this stage, but I couldn't find a way.
But maybe we can have a bit better check for the number. Can you do maybe Number.isInteger(markerIndex) && markerIndex >= 0?

canova · 2026-02-25T12:00:30Z

src/selectors/per-thread/markers.ts

   */
  const getSelectedMarkerIndex: Selector<MarkerIndex | null> = (state) =>
-    threadSelectors.getViewOptions(state).selectedMarker;
+    UrlState.getSelectedMarker(state, threadsKey);


Can we now do further validation here? Since we can get the markerList with getFullMarkerList now, we can add a check like this before returning the url state:

if (markerIndex === null || markerIndex >= markerList.length) { return null; }

canova · 2026-02-25T12:04:37Z

src/components/marker-chart/Canvas.tsx

+    const markerIndexToTimingRow = this._getMarkerIndexToTimingRow(
+      markerTimingAndBuckets
+    );
+    const rowIndex = markerIndexToTimingRow[selectedMarkerIndex];


Can we check if the rowIndex is found here? It can be undefined if we can't find it.

It was there before, I've removed it because Uint32Array (result of this._getMarkerIndexToTimingRow) will return 0 for non-existent items, not undefined. But there does not seem to be anything bad with it. If the selected marker is on a thread that was for example filtered out, we will just scroll to the very top on load.

It will still return undefined if the index is out of bounds, for example: new Uint32Array(4)[5]

Oh yeah, true

I understood why I had that impression. Without having noUncheckedIndexedAccess: true in tsconfig (which we don't have), ts language server and thus my IDE infers the type as number only, assuming a successful array lookup. But adding that flag seems to bring many errors, so I will just take that into account without adding any flags.

canova · 2026-02-25T12:33:54Z

src/components/shared/chart/Canvas.tsx

+  // Current vertical scroll offset of the viewport.
+  readonly viewportTop?: CssPixels;


Hmm, this looks a bit brittle. I don't think we should get the viewportTop from the parent component since it's actually an internal detail of the canvas.

Maybe we can replace both getTooltipPosition and viewportTop with something like this:

// Pre-computed canvas-relative offset for the tooltip of the selected item. readonly selectedItemTooltipOffset?: { offsetX: CssPixels; offsetY: CssPixels } | null;

And we always handle the selectedItemTooltipOffset calculation in the render of the parent component.
We can then use this value directly inside _syncSelectedItemFromProp. I think this might simplify a few things.

What do you think?

This indeed sounds better. I'll re-implement it that way, thanks for your suggestion.

canova · 2026-02-25T12:35:15Z

src/components/shared/chart/Canvas.tsx

+      // The canvas just received its dimensions for the first time (containerWidth
+      // went from 0 to non-zero). If a selectedItem was provided but couldn't be
+      // synced in componentDidMount, do it now.
+      if (
+        prevProps.containerWidth === 0 &&
+        this.props.containerWidth !== 0 &&
+        this.props.selectedItem !== undefined
+      ) {
+        this._syncSelectedItemFromProp(this.props.selectedItem);
+      }
+
+      // The viewport scrolled (e.g. to bring the selected item into view on
+      // load). Re-sync the tooltip position so it stays on the selected item.
+      if (
+        this.props.viewportTop !== undefined &&
+        this.props.viewportTop !== prevProps.viewportTop &&
+        this.props.selectedItem !== undefined
+      ) {
+        this._syncSelectedItemFromProp(this.props.selectedItem);
+      }
+


If we go with the selectedItemTooltipOffset solution, I would merge these both branches into one:

// Re-sync the tooltip position when the offset changes — this covers both // the initial viewport sizing (null → valid) and subsequent scrolls that // move the selected item. Only trigger when the item itself hasn't changed; // a changed item is already handled by the condition above. if ( this.props.selectedItemTooltipOffset !== prevProps.selectedItemTooltipOffset && this.props.selectedItem !== undefined && this.props.selectedItem === prevProps.selectedItem ) { this._syncSelectedItemFromProp(this.props.selectedItem ?? null); }

…ectedItemTooltipOffset Replace the getTooltipPosition callback and viewportTop props with a single pre-computed selectedItemTooltipOffset prop. The parent component (MarkerChartCanvas) now owns the full tooltip position calculation including the viewport bounds check, since it has direct access to all the layout data. This simplifies ChartCanvas by removing internal details it didn't need to know about: - _syncSelectedItemFromProp no longer calls a callback or checks viewport bounds; - The three separate componentDidUpdate trigger conditions collapse into one; - viewportTop is no longer leaked as a prop. Also fix scroll-to-selected-marker on load when the viewport resizes after the initial layout (e.g. flex containers settling), by re-running _scrollSelectionIntoView when containerHeight changes.

Merging 'main' into this branch introduced an additional `componentDidMount`. This commit merges them.

canova

Looks good to me, thanks!

One thing I noticed was that when we select a long duration marker and then scroll, the tooltip shifts position vertically. But this is still an improvement to its old behavior (not moving at all on scroll), so let's land this and see if we can improve that part later.

canova · 2026-02-27T15:10:06Z

src/components/marker-chart/Canvas.tsx

    );
    const rowIndex = markerIndexToTimingRow[selectedMarkerIndex];
+
+    if (rowIndex === undefined) return;


Nit: We always wrap the if branches with curly braces in this repo in general. It would be good to be consistent. Could you wrap this return statement as well?

(I thought we had a linter error for this, but maybe that went away over the time. I see a few of them slipped in the codebase. It might be good to revisit that later at some point)

I've created #5867

canova · 2026-02-27T15:24:53Z

src/components/marker-chart/Canvas.tsx

+    const offsetX = isInstantMarker ? x : x + Math.min(w / 3, 30);
+
+    // Step 5: Calculate vertical (Y) position
+    const offsetY: CssPixels = rowIndex * rowHeight - viewportTop + 5;


Hmm, what's + 5? Can you add a comment also

rowIndex * rowHeight - viewportTop gives the exact top of the row in canvas-relative coordinates; + 5 shifts it down a few pixels into the row body. I'll add a comment.

canova · 2026-02-27T15:26:30Z

src/components/shared/chart/Canvas.tsx

+          this.props.selectedItemTooltipOffset !==
+          prevProps.selectedItemTooltipOffset;
+
+        const alreadySetInternally =


Hmm, what's alreadySetInternally? Could be good to add a comment.

1. Use curly braces with conditionals 2. Add necessary comments

fatadel · 2026-02-27T20:27:16Z

Looks good to me, thanks!

One thing I noticed was that when we select a long duration marker and then scroll, the tooltip shifts position vertically. But this is still an improvement to its old behavior (not moving at all on scroll), so let's land this and see if we can improve that part later.

Thanks a lot for the review! Regarding this, could you please record a short video with what exactly you mean?

fatadel · 2026-02-27T20:28:25Z

@canova Once again, thanks for the elaborate review! I have addressed your comments, merging now 😌

* main: (289 commits) Escape CSS URLs that are coming from profiles (firefox-devtools#5874) Memoize the computation of search filtering across threads. Memoize the computation of the implementation-filtered thread across threads. Memoize the computation of the call node table across threads. Reserve functions for collapsed resources globally. Correctly adjust transforms when merging profiles. Use Int32Array and oldXToNewXPlusOne convention in merge-compare.ts. Make profile compacting apply to the newly-shared tables. Update snapshots. Share the stackTable, frameTable, funcTable, resourceTable and nativeSymbols across threads. Update docsify to v4.13.1 with a PR manually patched Remove unused ga docsify plugin Remove async attribute from module script tag. (firefox-devtools#5870) Bump rollup from 2.79.2 to 2.80.0 Remove unused defaultCategory parameter. Implement the "collapse resource" transform with the help of the "collapse direct recursion" transform. Persist selected marker in URL and show sticky tooltip on load (firefox-devtools#5847) Fix unhandled promise rejection in setupInitialUrlState (firefox-devtools#5864) Force canvas redraw when system theme changes (firefox-devtools#5861) Fix the color of dark mode back arrow svg (firefox-devtools#5863) ...

Changes: [fatadel] Fix crash when nativeSymbol index is out of bounds in assembly view (#5850) [depfu[bot]] Update all Yarn dependencies (2026-02-25) (#5859) [Nazım Can Altınova] Fix the color of dark mode back arrow svg (#5863) [fatadel] Force canvas redraw when system theme changes (#5861) [Nazım Can Altınova] Fix unhandled promise rejection in setupInitialUrlState (#5864) [fatadel] Persist selected marker in URL and show sticky tooltip on load (#5847) [Markus Stange] Implement the "collapse resource" transform with the help of the "collapse direct recursion" transform. (#5824) [Markus Stange] Bump rollup from 2.79.2 to 2.80.0 (#5868) [Markus Stange] Remove async attribute from module script tag. (#5870) [Nazım Can Altınova] Update the docsify package that's used in the user documentation (#5872) [Markus Stange] Share stackTable, frameTable, funcTable, resourceTable and nativeSymbols between threads (#5482) [Nazım Can Altınova] Escape CSS URLs that are coming from profiles (#5874) [fatadel] Update home page message for the other browser case (#5866) [fatadel] Add support for ternaries in marker labels (#5857) [Markus Stange] Reduce allocations for getStackLineInfo + getStackAddressInfo (#5761) And special thanks to our localizers: de: Ger fy-NL: Fjoerfoks it: Francesco Lodolo [:flod] nl: Fjoerfoks ru: berry ru: Valery Ledovskoy zh-TW: Pin-guang Chen

fatadel changed the title ~~Persist selected markers in the url~~ Persist the selected marker in the url Feb 17, 2026

fatadel marked this pull request as ready for review February 17, 2026 13:52

fatadel requested review from canova and mstange February 17, 2026 13:52

canova reviewed Feb 19, 2026

View reviewed changes

fatadel force-pushed the persist-selected-marker-5241 branch 2 times, most recently from 14671c0 to fff55d5 Compare February 19, 2026 15:55

fatadel marked this pull request as draft February 20, 2026 14:19

fatadel marked this pull request as ready for review February 23, 2026 15:24

fatadel added 11 commits February 23, 2026 16:25

persist selected marker

58f2081

make selected marker id part of MarkersQuery

b85d330

add tests

0fc3136

show sticky tooltip for selected marker on load

ea8a817

sync selected marker with browser history

e227fa2

initialize selected marker from VIEW_FULL_PROFILE instead of separate…

33174ba

… dispatches

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

3864ace

replace requestAnimationFrame with containerWidth check for tooltip s…

008fd2e

…ync on load

use SelectedMarkersPerThread type directly

a6f157a

scroll to selected marker, if not visible on load

680c730

refine comments

2990331

fatadel force-pushed the persist-selected-marker-5241 branch from 323de83 to 2990331 Compare February 23, 2026 15:26

fatadel requested a review from canova February 23, 2026 15:26

canova reviewed Feb 25, 2026

View reviewed changes

code review refinements

c4152c4

fatadel force-pushed the persist-selected-marker-5241 branch from 1259ed7 to c4152c4 Compare February 26, 2026 15:01

fatadel requested a review from canova February 26, 2026 17:38

Merge branch 'main' into persist-selected-marker-5241

7a763a1

Fix merge conflict

e93812c

Merging 'main' into this branch introduced an additional `componentDidMount`. This commit merges them.

mstange mentioned this pull request Feb 26, 2026

Share stackTable, frameTable, funcTable, resourceTable and nativeSymbols between threads #5482

Merged

8 tasks

canova approved these changes Feb 27, 2026

View reviewed changes

canova reviewed Feb 27, 2026

View reviewed changes

fatadel mentioned this pull request Feb 27, 2026

Add linter check for curly braces with conditional blocks #5867

Open

Address code review

946a4ab

1. Use curly braces with conditionals 2. Add necessary comments

fatadel merged commit c98c103 into firefox-devtools:main Feb 27, 2026
19 checks passed

fatadel deleted the persist-selected-marker-5241 branch February 27, 2026 20:44

canova mentioned this pull request Mar 3, 2026

Deploy March 3, 2026 #5877

Merged

		// Current vertical scroll offset of the viewport.
		readonly viewportTop?: CssPixels;

Conversation

fatadel commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

codecov bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canova Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatadel commented Feb 23, 2026

Uh oh!

canova left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canova Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatadel commented Feb 27, 2026

Uh oh!

fatadel commented Feb 27, 2026

Uh oh!

fatadel commented Feb 17, 2026 •

edited

Loading

codecov bot commented Feb 17, 2026 •

edited

Loading

canova Feb 19, 2026 •

edited

Loading

canova left a comment •

edited

Loading

canova Feb 27, 2026 •

edited

Loading