Skip to content

Set the preview selection to null when there is no selection #5568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mstange merged 4 commits into from
Aug 27, 2025
Merged

Set the preview selection to null when there is no selection #5568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2a7ec1
Add a selector called getPreviewSelectionIsBeingModified.
mstange Aug 15, 2025
1faf2e3
Remove hasSelection property.
mstange Aug 15, 2025
b04b39b
Make Draggable generic over the value type.
mstange Aug 15, 2025
118c6a4
Make _makeOnMove slightly easier to understand.
mstange Aug 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/actions/profile-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1749,13 +1749,17 @@ export function changeShowJsTracerSummary(
}

export function updatePreviewSelection(
previewSelection: PreviewSelection
previewSelection: PreviewSelection | null
): ThunkAction<void> {
return (dispatch, getState) => {
// Only dispatch if the selection changes. This function can fire in a tight loop,
// and this check saves a dispatch.
const currentPreviewSelection = getPreviewSelection(getState());
if (!objectShallowEquals(currentPreviewSelection, previewSelection)) {
// Only dispatch if the selection changes. This function can fire in a tight loop,
// and this check saves a dispatch.
if (
!currentPreviewSelection ||
!previewSelection ||
!objectShallowEquals(currentPreviewSelection, previewSelection)
) {
Comment on lines +1758 to +1762

@canova canova Aug 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mstange Aug 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dispatch({
type: 'UPDATE_PREVIEW_SELECTION',
previewSelection,
Expand Down
4 changes: 2 additions & 2 deletions src/components/app/BottomBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
getSourceViewCode,
getAssemblyViewCode,
} from 'firefox-profiler/selectors/code';
import { getPreviewSelection } from 'firefox-profiler/selectors/profile';
import { getPreviewSelectionIsBeingModified } from 'firefox-profiler/selectors/profile';
import explicitConnect from 'firefox-profiler/utils/connect';

import type { ConnectedProps } from 'firefox-profiler/utils/connect';
Expand Down Expand Up @@ -295,7 +295,7 @@ export const BottomBox = explicitConnect<{}, StateProps, DispatchProps>({
selectedNodeSelectors.getAssemblyViewAddressTimings(state),
assemblyViewScrollGeneration: getAssemblyViewScrollGeneration(state),
assemblyViewIsOpen: getAssemblyViewIsOpen(state),
disableOverscan: getPreviewSelection(state).isModifying,
disableOverscan: getPreviewSelectionIsBeingModified(state),
}),
mapDispatchToProps: {
closeBottomBox,
Expand Down
2 changes: 1 addition & 1 deletion src/components/app/ProfileFilterNavigator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ export const ProfileFilterNavigator = explicitConnect<
const items = getCommittedRangeLabels(state);
const previewSelection = getPreviewSelection(state);
const profileTimelineUnit = getProfileTimelineUnit(state);
const uncommittedItem = previewSelection.hasSelection
const uncommittedItem = previewSelection
? getFormattedTimelineValue(
previewSelection.selectionEnd - previewSelection.selectionStart,
profileTimelineUnit
Expand Down
4 changes: 2 additions & 2 deletions src/components/calltree/CallTree.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
import {
getScrollToSelectionGeneration,
getFocusCallTreeGeneration,
getPreviewSelection,
getPreviewSelectionIsBeingModified,
getCategories,
getCurrentTableViewOptions,
} from 'firefox-profiler/selectors/profile';
Expand Down Expand Up @@ -410,7 +410,7 @@ export const CallTree = explicitConnect<{}, StateProps, DispatchProps>({
expandedCallNodeIndexes:
selectedThreadSelectors.getExpandedCallNodeIndexes(state),
searchStringsRegExp: getSearchStringsAsRegExp(state),
disableOverscan: getPreviewSelection(state).isModifying,
disableOverscan: getPreviewSelectionIsBeingModified(state),
invertCallstack: getInvertCallstack(state),
implementationFilter: getImplementationFilter(state),
// Use the filtered call node max depth, rather than the preview filtered call node
Expand Down
2 changes: 1 addition & 1 deletion src/components/flame-graph/FlameGraph.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ type StateProps = {
readonly ctssSampleIndexOffset: number;
readonly maxStackDepthPlusOne: number;
readonly timeRange: StartEndRange;
readonly previewSelection: PreviewSelection;
readonly previewSelection: PreviewSelection | null;
readonly flameGraphTiming: FlameGraphTiming;
readonly callTree: CallTree;
readonly callNodeInfo: CallNodeInfo;
Expand Down
1 change: 0 additions & 1 deletion src/components/js-tracer/Canvas.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,6 @@ class JsTracerCanvasImpl extends React.PureComponent<Props, State> {
// const { markers, updatePreviewSelection } = this.props;
// const marker = markers[eventIndex];
// updatePreviewSelection({
// hasSelection: true,
// isModifying: false,
// selectionStart: marker.start,
// selectionEnd: marker.start + marker.dur,
Expand Down
2 changes: 1 addition & 1 deletion src/components/js-tracer/Chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type StateProps = {
readonly stringTable: StringTable;
readonly timeRange: StartEndRange;
readonly threadsKey: ThreadsKey;
readonly previewSelection: PreviewSelection;
readonly previewSelection: PreviewSelection | null;
};

type Props = ConnectedProps<OwnProps, StateProps, DispatchProps>;
Expand Down
1 change: 0 additions & 1 deletion src/components/marker-chart/Canvas.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,6 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
);

updatePreviewSelection({
hasSelection: true,
isModifying: false,
selectionStart: start,
selectionEnd: end,
Expand Down
2 changes: 1 addition & 1 deletion src/components/marker-chart/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ type StateProps = {
readonly markerListLength: number;
readonly timeRange: StartEndRange;
readonly threadsKey: ThreadsKey;
readonly previewSelection: PreviewSelection;
readonly previewSelection: PreviewSelection | null;
readonly rightClickedMarkerIndex: MarkerIndex | null;
readonly selectedMarkerIndex: MarkerIndex | null;
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/network-chart/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {

import {
getScrollToSelectionGeneration,
getPreviewSelection,
getPreviewSelectionIsBeingModified,
getPreviewSelectionRange,
} from '../../selectors/profile';
import { selectedThreadSelectors } from '../../selectors/per-thread';
Expand Down Expand Up @@ -422,7 +422,7 @@ export const NetworkChart = explicitConnect<
hoveredMarkerIndexFromState:
selectedThreadSelectors.getHoveredMarkerIndex(state),
timeRange: getPreviewSelectionRange(state),
disableOverscan: getPreviewSelection(state).isModifying,
disableOverscan: getPreviewSelectionIsBeingModified(state),
threadsKey: getSelectedThreadsKey(state),
}),
mapDispatchToProps: {
Expand Down
19 changes: 6 additions & 13 deletions src/components/shared/Draggable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,17 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import * as React from 'react';
import type { Milliseconds } from 'firefox-profiler/types';

export type OnMove = (
originalValue: {
readonly selectionEnd: Milliseconds;
readonly selectionStart: Milliseconds;
},
export type OnMove<T> = (
originalValue: T,
dx: number,
dy: number,
isModifying: boolean
) => void;

type Props = {
value: {
readonly selectionStart: Milliseconds;
readonly selectionEnd: Milliseconds;
};
onMove: OnMove;
type Props<T> = {
value: T;
onMove: OnMove<T>;
className: string;
children?: React.ReactNode;
};
Expand All @@ -37,7 +30,7 @@ type State = {
* x and y deltas compared to the mouse position at mousedown.
* During the drag, the additional className 'dragging' is set on the element.
*/
export class Draggable extends React.PureComponent<Props, State> {
export class Draggable<T> extends React.PureComponent<Props<T>, State> {
_container: HTMLDivElement | null = null;
_handlers: {
mouseMoveHandler: (param: MouseEvent) => void;
Expand Down
13 changes: 5 additions & 8 deletions src/components/shared/MarkerContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ type OwnProps = {
type StateProps = {
readonly marker: Marker;
readonly markerIndex: MarkerIndex;
readonly previewSelection: PreviewSelection;
readonly previewSelection: PreviewSelection | null;
readonly committedRange: StartEndRange;
readonly thread: Thread | null;
readonly implementationFilter: ImplementationFilter;
Expand All @@ -74,12 +74,11 @@ class MarkerContextMenuImpl extends PureComponent<Props> {
const { updatePreviewSelection, previewSelection, committedRange } =
this.props;

const selectionEnd = previewSelection.hasSelection
const selectionEnd = previewSelection
? previewSelection.selectionEnd
: committedRange.end;

updatePreviewSelection({
hasSelection: true,
isModifying: false,
selectionStart,
selectionEnd,
Expand All @@ -90,7 +89,7 @@ class MarkerContextMenuImpl extends PureComponent<Props> {
const { updatePreviewSelection, committedRange, previewSelection } =
this.props;

const selectionStart = previewSelection.hasSelection
const selectionStart = previewSelection
? previewSelection.selectionStart
: committedRange.start;

Expand All @@ -102,7 +101,6 @@ class MarkerContextMenuImpl extends PureComponent<Props> {
}

updatePreviewSelection({
hasSelection: true,
isModifying: false,
selectionStart,
selectionEnd,
Expand Down Expand Up @@ -137,7 +135,6 @@ class MarkerContextMenuImpl extends PureComponent<Props> {
}

updatePreviewSelection({
hasSelection: true,
isModifying: false,
selectionStart: marker.start,
selectionEnd: marker.end,
Expand Down Expand Up @@ -385,11 +382,11 @@ class MarkerContextMenuImpl extends PureComponent<Props> {
const { marker, previewSelection, committedRange } = this.props;
const { data } = marker;

const selectionEnd = previewSelection.hasSelection
const selectionEnd = previewSelection
? previewSelection.selectionEnd
: committedRange.end;

const selectionStart = previewSelection.hasSelection
const selectionStart = previewSelection
? previewSelection.selectionStart
: committedRange.start;

Expand Down
22 changes: 7 additions & 15 deletions src/components/shared/chart/Viewport.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ type ViewportProps<ChartProps> = {
// The "committed range", whose endpoints correspond to 0 and 1.
readonly timeRange: StartEndRange;
// The preview selection, whose endpoints correspond to viewportLeft and viewportRight.
readonly previewSelection: PreviewSelection;
readonly previewSelection: PreviewSelection | null;
// The left margin. Margins are outside the viewport but inside containerWidth.
readonly marginLeft: CssPixels;
// The right margin. Margins are outside the viewport but inside containerWidth.
Expand Down Expand Up @@ -214,7 +214,7 @@ class ChartViewportImpl<OwnProps> extends React.PureComponent<
> {
zoomScrollId: number = 0;
_pendingPreviewSelectionUpdates: Array<
(horizontalViewport: HorizontalViewport) => PreviewSelection
(horizontalViewport: HorizontalViewport) => PreviewSelection | null
> = [];
_container: HTMLDivElement | null = null;
_takeContainerRef = (container: HTMLDivElement) => {
Expand All @@ -235,10 +235,10 @@ class ChartViewportImpl<OwnProps> extends React.PureComponent<
}

getHorizontalViewport(
previewSelection: PreviewSelection,
previewSelection: PreviewSelection | null,
timeRange: StartEndRange
) {
if (previewSelection.hasSelection) {
if (previewSelection) {
const { selectionStart, selectionEnd } = previewSelection;
const timeRangeLength = timeRange.end - timeRange.start;
return {
Expand Down Expand Up @@ -433,7 +433,7 @@ class ChartViewportImpl<OwnProps> extends React.PureComponent<
* processes all queued updates from a requestAnimationFrame callback.
*/
_addBatchedPreviewSelectionUpdate(
callback: (param: HorizontalViewport) => PreviewSelection
callback: (param: HorizontalViewport) => PreviewSelection | null
) {
if (this._pendingPreviewSelectionUpdates.length === 0) {
requestAnimationFrame(() => this._flushPendingPreviewSelectionUpdates());
Expand Down Expand Up @@ -532,14 +532,10 @@ class ChartViewportImpl<OwnProps> extends React.PureComponent<
viewportProps: { timeRange },
} = this.props;
if (newViewportLeft === 0 && newViewportRight === 1) {
return {
hasSelection: false,
isModifying: false,
};
return null;
}
const timeRangeLength = timeRange.end - timeRange.start;
return {
hasSelection: true,
isModifying: false,
selectionStart: timeRange.start + timeRangeLength * newViewportLeft,
selectionEnd: timeRange.start + timeRangeLength * newViewportRight,
Expand Down Expand Up @@ -722,10 +718,7 @@ class ChartViewportImpl<OwnProps> extends React.PureComponent<
// Calculate left and right in terms of the unit interval of the profile range.
const viewportLength = viewportRight - viewportLeft;
if (viewportLength >= 1) {
return {
hasSelection: false,
isModifying: false,
};
return null;
}
const viewportPixelWidth = containerWidth - marginLeft - marginRight;
const unitOffsetX = offsetX * (viewportLength / viewportPixelWidth);
Expand All @@ -742,7 +735,6 @@ class ChartViewportImpl<OwnProps> extends React.PureComponent<

const timeRangeLength = timeRange.end - timeRange.start;
return {
hasSelection: true,
isModifying: false,
selectionStart: timeRange.start + timeRangeLength * newViewportLeft,
selectionEnd: timeRange.start + timeRangeLength * newViewportRight,
Expand Down
1 change: 0 additions & 1 deletion src/components/stack-chart/Canvas.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,6 @@ class StackChartCanvasImpl extends React.PureComponent<Props> {
const { depth, stackTimingIndex } = hoveredItem;
const { combinedTimingRows, updatePreviewSelection } = this.props;
updatePreviewSelection({
hasSelection: true,
isModifying: false,
selectionStart: combinedTimingRows[depth].start[stackTimingIndex],
selectionEnd: combinedTimingRows[depth].end[stackTimingIndex],
Expand Down
2 changes: 1 addition & 1 deletion src/components/stack-chart/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ type StateProps = {
readonly sameWidthsIndexToTimestampMap: SameWidthsIndexToTimestampMap;
readonly timeRange: StartEndRange;
readonly interval: Milliseconds;
readonly previewSelection: PreviewSelection;
readonly previewSelection: PreviewSelection | null;
readonly threadsKey: ThreadsKey;
readonly callNodeInfo: CallNodeInfo;
readonly categories: CategoryList;
Expand Down
12 changes: 6 additions & 6 deletions src/components/timeline/Markers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { Tooltip } from 'firefox-profiler/components/tooltip/Tooltip';
import { TooltipMarker } from 'firefox-profiler/components/tooltip/Marker';
import { timeCode } from 'firefox-profiler/utils/time-code';
import explicitConnect from 'firefox-profiler/utils/connect';
import { getPreviewSelection } from 'firefox-profiler/selectors/profile';
import { getPreviewSelectionIsBeingModified } from 'firefox-profiler/selectors/profile';
import { getThreadSelectorsFromThreadsKey } from 'firefox-profiler/selectors/per-thread';
import { getSelectedThreadIndexes } from 'firefox-profiler/selectors/url-state';
import { changeRightClickedMarker } from 'firefox-profiler/actions/profile-view';
Expand Down Expand Up @@ -528,7 +528,7 @@ export const TimelineMarkersJank = explicitConnect<
// These don't use marker schema as they are derived.
markerIndexes: selectors.getTimelineJankMarkerIndexes(state),
isSelected: _getTimelineMarkersIsSelected(selectedThreads, threadsKey),
isModifyingSelection: getPreviewSelection(state).isModifying,
isModifyingSelection: getPreviewSelectionIsBeingModified(state),
testId: 'TimelineMarkersJank',
rightClickedMarker: selectors.getRightClickedMarker(state),
};
Expand Down Expand Up @@ -557,7 +557,7 @@ export const TimelineMarkersOverview = explicitConnect<
getMarker: selectors.getMarkerGetter(state),
markerIndexes: selectors.getTimelineOverviewMarkerIndexes(state),
isSelected: _getTimelineMarkersIsSelected(selectedThreads, threadsKey),
isModifyingSelection: getPreviewSelection(state).isModifying,
isModifyingSelection: getPreviewSelectionIsBeingModified(state),
testId: 'TimelineMarkersOverview',
rightClickedMarker: selectors.getRightClickedMarker(state),
};
Expand All @@ -583,7 +583,7 @@ export const TimelineMarkersFileIo = explicitConnect<
getMarker: selectors.getMarkerGetter(state),
markerIndexes: selectors.getTimelineFileIoMarkerIndexes(state),
isSelected: _getTimelineMarkersIsSelected(selectedThreads, threadsKey),
isModifyingSelection: getPreviewSelection(state).isModifying,
isModifyingSelection: getPreviewSelectionIsBeingModified(state),
testId: 'TimelineMarkersFileIo',
rightClickedMarker: selectors.getRightClickedMarker(state),
};
Expand All @@ -609,7 +609,7 @@ export const TimelineMarkersMemory = explicitConnect<
getMarker: selectors.getMarkerGetter(state),
markerIndexes: selectors.getTimelineMemoryMarkerIndexes(state),
isSelected: _getTimelineMarkersIsSelected(selectedThreads, threadsKey),
isModifyingSelection: getPreviewSelection(state).isModifying,
isModifyingSelection: getPreviewSelectionIsBeingModified(state),
additionalClassName: 'timelineMarkersMemory',
testId: 'TimelineMarkersMemory',
rightClickedMarker: selectors.getRightClickedMarker(state),
Expand All @@ -636,7 +636,7 @@ export const TimelineMarkersIPC = explicitConnect<
getMarker: selectors.getMarkerGetter(state),
markerIndexes: selectors.getTimelineIPCMarkerIndexes(state),
isSelected: _getTimelineMarkersIsSelected(selectedThreads, threadsKey),
isModifyingSelection: getPreviewSelection(state).isModifying,
isModifyingSelection: getPreviewSelectionIsBeingModified(state),
additionalClassName: 'timelineMarkersIPC',
testId: 'TimelineMarkersIPC',
rightClickedMarker: selectors.getRightClickedMarker(state),
Expand Down
Loading