Persist the selected marker in the url

[fatadel]

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]

Codecov Report

❌ Patch coverage is 57.65766% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.46%. Comparing base (37b31b0) to head (2990331).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/marker-chart/Canvas.tsx	36.17%	30 Missing ⚠️
src/components/shared/chart/Canvas.tsx	54.05%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5847      +/-   ##
==========================================
- Coverage   85.56%   85.46%   -0.11%     
==========================================
  Files         319      319              
  Lines       31420    31525     +105     
  Branches     8661     8711      +50     
==========================================
+ Hits        26885    26943      +58     
- Misses       4104     4151      +47     
  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]

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?

[fatadel]

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?

[fatadel]

(resolved irl)

[canova]

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.

[fatadel]

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.

[canova]

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:

1. In componentDidMount: Call _syncSelectedItemFromProp directly, but only if containerWidth !== 0. If it's still 0, skip it, componentDidUpdate will pick it up.
2. 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!

[fatadel]

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

[canova]

Nit: We can use the SelectedMarkersPerThread type here directly.

[fatadel]

@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 :)