Set the preview selection to null when there is no selection

[mstange]

We are currently using a "no preview selection" object of the shape { hasSelection: false, isModifying: false } when there is no preview selection. We can instead just use null, it's more idiomatic.

I recommend reviewing commits individually.

[codecov]

Codecov Report

❌ Patch coverage is 69.81132% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.83%. Comparing base (6444d1f) to head (ee22ac9).

Files with missing lines	Patch %	Lines
src/components/timeline/Selection.tsx	27.27%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5568      +/-   ##
==========================================
- Coverage   85.84%   85.83%   -0.02%     
==========================================
  Files         309      309              
  Lines       30338    30349      +11     
  Branches     8346     8348       +2     
==========================================
+ Hits        26045    26050       +5     
- Misses       3873     3879       +6     
  Partials      420      420              

☔ 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]

It looks much better indeed! Thanks!

Nit: I think this previewSelection can be just PreviewSelection now:

profiler/src/components/timeline/Selection.tsx

Lines 343 to 349 in fb9a4bc

	renderSelectionOverlay(previewSelection: {
	readonly selectionStart: number;
	readonly selectionEnd: number;
	readonly isModifying: boolean;
	readonly draggingStart?: boolean;
	readonly draggingEnd?: boolean;
	}) {

[canova]

I hope no one will pass the same object with the mutated value here. We know we should always pass a new object inside updatePreviewSelection and never mutate, but should we make this assumption clearer with a function comment, so other people in the future don't use it incorrectly?

[mstange]

Ah true, I'm not sure why I thought that check was worth adding. I've just inlined-away this function again and removed that strict equality check.

[mstange]

Nit: I think this previewSelection can be just PreviewSelection now:

Oops, of course! I think that was my initial motivation for making this change, and then I just forgot about it! Thanks for catching it.

[canova]

Hm, I think this new if check is not 100% correct.

We have:

!currentPreviewSelection ||
!previewSelection)

This means that when either currentPreviewSelection or previewSelection is null, it will always dispatch UPDATE_PREVIEW_SELECTION, including the cases where they are both null.

This strict equality check was still needed for the case where they are both null. Probably that's why you initially wanted to have a different function so it's easier to do early return.

Maybe something like:

    if (
      (currentPreviewSelection === null) !== (previewSelection === null) ||
      (currentPreviewSelection &&
        previewSelection &&
        !objectShallowEquals(currentPreviewSelection, previewSelection))
    ) 

[mstange]

Yes, what I landed is not equivalent to the check we had before this PR, but that just means that we may dispatch the action unnecessarily - this may be slower but it's not a "bug". My thinking was that we probably don't call it in a loop when the selection is null. But I haven't verified this.

Looks like the check was initially added in #1481 for the case where you're "zooming out" on a viewport when you're already fully zoomed out.

Ok, checking now, we will indeed repeatedly dispatch UPDATE_PREVIEW_SELECTION with null in that case.

Oh, but this doesn't actually change the identity of the redux state object! The previewSelection reducer returns null for its new state when processing this action, which is the same as its previous state, so the viewOptions reducer (from combineReducers) doesn't recreate the outer object.

Ok so I think we can keep it as-is.