Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 29 additions & 1 deletion src/app-logic/url-handling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import type {
NativeSymbolInfo,
Transform,
IndexIntoFrameTable,
MarkerIndex,
SelectedMarkersPerThread,
} from 'firefox-profiler/types';
import {
decodeUintArrayFromUrlComponent,
Expand All @@ -49,7 +51,7 @@ import {
import { tabSlugs } from '../app-logic/tabs-handling';
import { StringTable } from 'firefox-profiler/utils/string-table';

export const CURRENT_URL_VERSION = 13;
export const CURRENT_URL_VERSION = 14;

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.

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


/**
* This static piece of state might look like an anti-pattern, but it's a relatively
Expand Down Expand Up @@ -184,6 +186,7 @@ type CallTreeQuery = BaseQuery & {

type MarkersQuery = BaseQuery & {
markerSearch: string; // "DOMEvent"
marker?: MarkerIndex; // Selected marker index for the current thread, e.g. 42
};

type NetworkQuery = BaseQuery & {
Expand Down Expand Up @@ -218,6 +221,7 @@ type Query = BaseQuery & {

// Markers specific
markerSearch?: string;
marker?: MarkerIndex;

// Network specific
networkSearch?: string;
Expand Down Expand Up @@ -367,11 +371,17 @@ export function getQueryStringFromUrlState(urlState: UrlState): string {
query = baseQuery as MarkersQueryShape;
query.markerSearch =
urlState.profileSpecific.markersSearchString || undefined;
query.marker =

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.

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

selectedThreadsKey !== null &&
urlState.profileSpecific.selectedMarkers[selectedThreadsKey] !== null
? urlState.profileSpecific.selectedMarkers[selectedThreadsKey]
: undefined;
break;
case 'network-chart':
query = baseQuery as NetworkQueryShape;
query.networkSearch =
urlState.profileSpecific.networkSearchString || undefined;
// TODO: Add support for query.marker
break;
case 'js-tracer': {
query = baseQuery as JsTracerQueryShape;
Expand Down Expand Up @@ -499,6 +509,19 @@ export function stateFromLocation(
transforms[selectedThreadsKey] = parseTransforms(query.transforms);
}

// Parse the selected marker for the current thread
const selectedMarkers: SelectedMarkersPerThread = {};
if (
selectedThreadsKey !== null &&
query.marker !== undefined &&
query.marker !== null
) {
const markerIndex = Number(query.marker);
if (Number.isInteger(markerIndex) && markerIndex >= 0) {
selectedMarkers[selectedThreadsKey] = markerIndex;

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.

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?

}
}

// tabID is used for the tab selector that we have in our full view.
let tabID = null;
if (query.tabID && Number.isInteger(Number(query.tabID))) {
Expand Down Expand Up @@ -587,6 +610,7 @@ export function stateFromLocation(
legacyHiddenThreads: query.hiddenThreads
? query.hiddenThreads.split('-').map((index) => Number(index))
: null,
selectedMarkers,
},
};
}
Expand Down Expand Up @@ -1196,6 +1220,10 @@ const _upgraders: {
[13]: (_) => {
// just added the focus-self transform
},
[14]: (_) => {
// Added marker parameter for persisting highlighted markers in URLs.
// This is backward compatible as the marker parameter is optional.
},
};

for (let destVersion = 1; destVersion <= CURRENT_URL_VERSION; destVersion++) {
Expand Down
137 changes: 137 additions & 0 deletions src/components/marker-chart/Canvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,49 @@ function getDefaultMarkerColors(isHighlighted: boolean) {
class MarkerChartCanvasImpl extends React.PureComponent<Props> {
_textMeasurement: TextMeasurement | null = null;

override componentDidUpdate(prevProps: Props) {
const viewportDidMount =
!prevProps.viewport.isSizeSet && this.props.viewport.isSizeSet;
const viewportResized =
this.props.viewport.isSizeSet &&
this.props.viewport.containerHeight !==
prevProps.viewport.containerHeight;
const selectedMarkerChanged =
this.props.selectedMarkerIndex !== prevProps.selectedMarkerIndex;

if (viewportDidMount || viewportResized || selectedMarkerChanged) {
this._scrollSelectionIntoView();
}
}

_scrollSelectionIntoView = () => {
const { selectedMarkerIndex, markerTimingAndBuckets, rowHeight, viewport } =
this.props;

if (selectedMarkerIndex === null) {
return;
}

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

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.

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

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.

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.

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.

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

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.

Oh yeah, true

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.

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.


if (rowIndex === undefined) {
return;
}

const y: CssPixels = rowIndex * rowHeight;
const { viewportTop, viewportBottom } = viewport;

if (y < viewportTop || y + rowHeight > viewportBottom) {
// Scroll the marker to the vertical center of the viewport.
const viewportHeight = viewportBottom - viewportTop;
const targetViewportTop = y + rowHeight / 2 - viewportHeight / 2;
viewport.moveViewport(0, viewportTop - targetViewportTop);
}
};

/**
* Get the fill, stroke, and text colors for a marker based on its schema and data.
* If the marker schema has a colorField, use that field's value.
Expand Down Expand Up @@ -957,8 +1000,100 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
);
};

/**
* Compute canvas-relative tooltip offset for the selected marker.
* Returns null if the marker can't be located in the timing data or
* is outside the visible viewport.
*/
_getSelectedItemTooltipOffset(): {
offsetX: CssPixels;
offsetY: CssPixels;
} | null {
const { selectedMarkerIndex } = this.props;
if (selectedMarkerIndex === null) {
return null;
}

const {
rangeStart,
rangeEnd,
markerTimingAndBuckets,
rowHeight,
marginLeft,
marginRight,
viewport: {
containerWidth,
containerHeight,
viewportLeft,
viewportRight,
viewportTop,
},
} = this.props;

// Step 1: Find which row this marker is displayed in
const markerIndexToTimingRow = this._getMarkerIndexToTimingRow(
markerTimingAndBuckets
);
const rowIndex = markerIndexToTimingRow[selectedMarkerIndex];

// Step 2: Get the timing data for all markers in this row
const markerTiming = markerTimingAndBuckets[rowIndex];
if (!markerTiming || typeof markerTiming === 'string') {
// Row is empty or is a bucket label (string), not actual marker data
return null;
}

// Step 3: Find the position of our specific marker within this row's data
let markerTimingIndex = -1;
for (let i = 0; i < markerTiming.length; i++) {
if (markerTiming.index[i] === selectedMarkerIndex) {
markerTimingIndex = i;
break;
}
}

if (markerTimingIndex === -1) {
return null;
}

// Step 4: Calculate horizontal (X) position
const startTimestamp = markerTiming.start[markerTimingIndex];
const endTimestamp = markerTiming.end[markerTimingIndex];

const markerContainerWidth = containerWidth - marginLeft - marginRight;
const rangeLength: Milliseconds = rangeEnd - rangeStart;
const viewportLength: UnitIntervalOfProfileRange =
viewportRight - viewportLeft;
const startTime: UnitIntervalOfProfileRange =
(startTimestamp - rangeStart) / rangeLength;
const endTime: UnitIntervalOfProfileRange =
(endTimestamp - rangeStart) / rangeLength;

const x: CssPixels =
((startTime - viewportLeft) * markerContainerWidth) / viewportLength +
marginLeft;
const w: CssPixels =
((endTime - startTime) * markerContainerWidth) / viewportLength;

// For instant markers (start === end), use the center point.
// For interval markers, use a point 1/3 into the marker (or 30px, whichever is smaller).
const isInstantMarker = startTimestamp === endTimestamp;
const offsetX = isInstantMarker ? x : x + Math.min(w / 3, 30);

// Step 5: Calculate vertical (Y) position
// + 5 offsets the tooltip slightly below the row's top edge.
const offsetY: CssPixels = rowIndex * rowHeight - viewportTop + 5;

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.

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

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.

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.


if (offsetY < 0 || offsetY > containerHeight) {
return null;
}

return { offsetX, offsetY };
}

override render() {
const { containerWidth, containerHeight, isDragging } = this.props.viewport;
const { selectedMarkerIndex } = this.props;

return (
<ChartCanvas
Expand All @@ -976,6 +1111,8 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
onMouseMove={this.onMouseMove}
onMouseLeave={this.onMouseLeave}
stickyTooltips={true}
selectedItem={selectedMarkerIndex}
selectedItemTooltipOffset={this._getSelectedItemTooltipOffset()}
/>
);
}
Expand Down
70 changes: 70 additions & 0 deletions src/components/shared/chart/Canvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ type Props<Item> = {
readonly onMouseLeave?: (e: { nativeEvent: MouseEvent }) => unknown;
// Defaults to false. Set to true if the chart should persist the tooltips on click.
readonly stickyTooltips?: boolean;
// Selected item coming from URL or other non-click mechanism.
readonly selectedItem?: Item | null;
// Pre-computed canvas-relative offset for the tooltip of the selected item
// (null when the item is not currently visible in the viewport).
readonly selectedItemTooltipOffset?: {
offsetX: CssPixels;
offsetY: CssPixels;
} | null;
};

// The naming of the X and Y coordinates here correspond to the ones
Expand Down Expand Up @@ -359,6 +367,28 @@ export class ChartCanvas<Item> extends React.Component<
this._canvas = canvas;
};

_syncSelectedItemFromProp = () => {
const { selectedItem, selectedItemTooltipOffset } = this.props;

if (selectedItem === undefined || selectedItem === null) {
this.setState({ selectedItem: null });
return;
}

if (!selectedItemTooltipOffset || !this._canvas) {
// Keep selectedItem set, so the tooltip stays at
// its last known position rather than disappearing.
this.setState({ selectedItem });
return;
}

const { offsetX, offsetY } = selectedItemTooltipOffset;
const canvasRect = this._canvas.getBoundingClientRect();
const pageX = canvasRect.left + window.scrollX + offsetX;
const pageY = canvasRect.top + window.scrollY + offsetY;
this.setState({ selectedItem, pageX, pageY });
};

override UNSAFE_componentWillReceiveProps() {
// It is possible that the data backing the chart has been
// changed, for instance after symbolication. Clear the
Expand All @@ -377,6 +407,15 @@ export class ChartCanvas<Item> extends React.Component<

override componentDidMount() {
window.addEventListener('profiler-theme-change', this._onThemeChange);

// If the viewport hasn't been laid out yet,
// componentDidUpdate will pick it up once it becomes non-zero.
if (
this.props.selectedItem !== undefined &&
this.props.containerWidth !== 0
) {
this._syncSelectedItemFromProp();
}
}

override componentWillUnmount() {
Expand All @@ -389,6 +428,37 @@ export class ChartCanvas<Item> extends React.Component<

override componentDidUpdate(prevProps: Props<Item>, prevState: State<Item>) {
if (prevProps !== this.props) {
if (this.props.selectedItem !== undefined) {
const selectedItemChanged =
this.props.selectedItem !== prevProps.selectedItem;
const canvasJustGotSize =
prevProps.containerWidth === 0 && this.props.containerWidth !== 0;
const offsetChanged =
this.props.selectedItemTooltipOffset !==
prevProps.selectedItemTooltipOffset;

// The item was already set internally (eg, via click handler),
// so skip the prop-driven sync which would overwrite that position.
const alreadySetInternally =

@canova canova Feb 27, 2026

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.

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

selectedItemChanged &&
hoveredItemsAreEqual(
this.state.selectedItem,
this.props.selectedItem
);

const offsetSyncRelevant =
offsetChanged &&
!selectedItemChanged &&
this.state.selectedItem !== null;

if (
!alreadySetInternally &&
(selectedItemChanged || canvasJustGotSize || offsetSyncRelevant)
) {
this._syncSelectedItemFromProp();
}
}

if (
this.state.selectedItem !== null &&
prevState.selectedItem === this.state.selectedItem
Expand Down
5 changes: 0 additions & 5 deletions src/reducers/profile-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ export const defaultThreadViewOptions: ThreadViewOptions = {
selectedInvertedCallNodePath: [],
expandedNonInvertedCallNodePaths: new PathSet(),
expandedInvertedCallNodePaths: new PathSet(),
selectedMarker: null,
selectedNetworkMarker: null,
lastSeenTransformCount: 0,
};
Expand Down Expand Up @@ -328,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, {
Expand Down
Loading