Scale custom marker graphs on values within the committed range. (#5587)

fqueze · web-flow · commit d3dfa050655b · 2025-09-10T19:42:42.000Z
diff --git a/src/components/timeline/TrackCustomMarkerGraph.tsx b/src/components/timeline/TrackCustomMarkerGraph.tsx
@@ -29,6 +29,7 @@ import type {
   IndexIntoStringTable,
   MarkerSchema,
   CollectedCustomMarkerSamples,
+  ValueBounds,
   MarkerGraphType,
   MarkerIndex,
   Marker,
@@ -51,6 +52,7 @@ type CanvasProps = {
   readonly markerSchema: MarkerSchema;
   readonly markerSampleRanges: [IndexIntoSamplesTable, IndexIntoSamplesTable];
   readonly collectedSamples: CollectedCustomMarkerSamples;
+  readonly valueBounds: ValueBounds;
   readonly width: CssPixels;
   readonly height: CssPixels;
   readonly getMarker: (param: MarkerIndex) => Marker;
@@ -101,6 +103,7 @@ class TrackCustomMarkerCanvas extends React.PureComponent<CanvasProps> {
       markerSchema,
       markerSampleRanges,
       collectedSamples,
+      valueBounds,
       height,
       width,
       getMarker,
@@ -136,7 +139,7 @@ class TrackCustomMarkerCanvas extends React.PureComponent<CanvasProps> {
       throw new Error('No lines for marker ' + name);
     }
 
-    const { minNumber, maxNumber } = collectedSamples;
+    const { minNumber, maxNumber } = valueBounds;
     const [sampleStart, sampleEnd] = markerSampleRanges;
 
     {
@@ -336,6 +339,7 @@ type StateProps = {
   readonly rangeEnd: Milliseconds;
   readonly markerSampleRanges: [IndexIntoSamplesTable, IndexIntoSamplesTable];
   readonly collectedSamples: CollectedCustomMarkerSamples;
+  readonly valueBounds: ValueBounds;
   readonly getMarker: (param: MarkerIndex) => Marker;
 };
 
@@ -484,6 +488,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {
       graphHeight,
       width,
       collectedSamples,
+      valueBounds,
       getMarker,
     } = this.props;
 
@@ -506,7 +511,8 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {
 
     const left = (width * (sampleTime - rangeStart)) / rangeLength;
 
-    const { minNumber, maxNumber, numbersPerLine } = collectedSamples;
+    const { minNumber, maxNumber } = valueBounds;
+    const { numbersPerLine } = collectedSamples;
 
     const dots = [];
 
@@ -572,6 +578,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {
       graphHeight,
       width,
       collectedSamples,
+      valueBounds,
       getMarker,
     } = this.props;
 
@@ -589,6 +596,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {
           height={graphHeight}
           width={width}
           collectedSamples={collectedSamples}
+          valueBounds={valueBounds}
           getMarker={getMarker}
         />
         {hoveredCounter === null ? null : (
@@ -620,6 +628,8 @@ export const TrackCustomMarkerGraph = explicitConnect<
         markerTrackSelectors.getCommittedRangeMarkerSampleRange(state),
       collectedSamples:
         markerTrackSelectors.getCollectedCustomMarkerSamples(state),
+      valueBounds:
+        markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(state),
       rangeStart: start,
       rangeEnd: end,
       getMarker: selectors.getMarkerGetter(state),
diff --git a/src/selectors/per-thread/markers.ts b/src/selectors/per-thread/markers.ts
@@ -27,6 +27,7 @@ import type {
   ThreadsKey,
   Tid,
   CollectedCustomMarkerSamples,
+  ValueBounds,
   IndexIntoSamplesTable,
   IndexIntoStringTable,
   State,
@@ -638,8 +639,6 @@ export function getMarkerSelectorsPerThread(
             );
           }
           const markerIndexes: MarkerIndex[] = [];
-          let minNumber = Infinity;
-          let maxNumber = -Infinity;
           const numbersPerLine: number[][] = [];
           const { graphs, name: schemaName } = markerSchema;
           const keys = graphs.map((graph) => {
@@ -659,19 +658,11 @@ export function getMarkerSelectorsPerThread(
               for (let i = 0; i < keys.length; ++i) {
                 const val = (data as any)[keys[i]];
                 numbersPerLine[i].push(val);
-                if (val < minNumber) {
-                  minNumber = val;
-                }
-                if (val > maxNumber) {
-                  maxNumber = val;
-                }
               }
             }
           });
 
           return {
-            minNumber,
-            maxNumber,
             numbersPerLine,
             markerIndexes,
           };
@@ -695,9 +686,50 @@ export function getMarkerSelectorsPerThread(
         )
     );
 
+    const getCommittedRangeMarkerSampleValueBounds: Selector<ValueBounds> =
+      createSelector(
+        getCollectedCustomMarkerSamples,
+        getCommittedRangeMarkerSampleRange,
+        (collectedSamples, sampleRange) => {
+          const [sampleStart, sampleEnd] = sampleRange;
+          const { numbersPerLine } = collectedSamples;
+
+          // Handle edge case where there are no samples in range
+          if (sampleStart >= sampleEnd) {
+            return { minNumber: 0, maxNumber: 0 };
+          }
+
+          let minNumber = Infinity;
+          let maxNumber = -Infinity;
+
+          for (
+            let sampleIndex = sampleStart;
+            sampleIndex < sampleEnd;
+            sampleIndex++
+          ) {
+            for (
+              let graphIndex = 0;
+              graphIndex < numbersPerLine.length;
+              graphIndex++
+            ) {
+              const val = numbersPerLine[graphIndex][sampleIndex];
+              if (val < minNumber) {
+                minNumber = val;
+              }
+              if (val > maxNumber) {
+                maxNumber = val;
+              }
+            }
+          }
+
+          return { minNumber, maxNumber };
+        }
+      );
+
     return {
       getCollectedCustomMarkerSamples,
       getCommittedRangeMarkerSampleRange,
+      getCommittedRangeMarkerSampleValueBounds,
     };
   }
 
diff --git a/src/test/components/TrackCustomMarker.test.tsx b/src/test/components/TrackCustomMarker.test.tsx
@@ -6,9 +6,11 @@
 
 import type { CssPixels } from 'firefox-profiler/types';
 import { Provider } from 'react-redux';
-import { fireEvent } from '@testing-library/react';
+import { fireEvent, act } from '@testing-library/react';
 
 import { render } from 'firefox-profiler/test/fixtures/testing-library';
+import { commitRange } from 'firefox-profiler/actions/profile-view';
+import { getThreadSelectors } from 'firefox-profiler/selectors/per-thread';
 import { TrackCustomMarker } from '../../components/timeline/TrackCustomMarker';
 import { ensureExists } from '../../utils/types';
 
@@ -46,9 +48,13 @@ function getMarkerPixelPosition(time: number): CssPixels {
   return (time * GRAPH_WIDTH) / SAMPLE_COUNT;
 }
 
-function setup() {
+function setup(
+  values: number[] = Array(SAMPLE_COUNT)
+    .fill(0)
+    .map((_, i) => i)
+) {
   const { profile, stringTable } = getProfileFromTextSamples(
-    Array(SAMPLE_COUNT).fill('A').join('  ')
+    Array(values.length).fill('A').join('  ')
   );
   const markerStringIndex = stringTable.indexForString('Marker');
   const threadIndex = 0;
@@ -83,9 +89,10 @@ function setup() {
     thread.markers.category.push(4);
     thread.markers.length++;
   };
-  for (let i = 0; i < SAMPLE_COUNT; i++) {
-    addMarker(i, i, i * 2);
-  }
+
+  values.forEach((value, index) => {
+    addMarker(index, value, value * 2);
+  });
   const store = storeWithProfile(profile);
   const { getState, dispatch } = store;
   const flushRafCalls = mockRaf();
@@ -142,6 +149,7 @@ function setup() {
     flushRafCalls,
     getMarkerDot,
     getContextDrawCalls,
+    markerStringIndex,
   };
 }
 
@@ -300,3 +308,89 @@ describe('TrackCustomMarker with intersection observer', function () {
     );
   });
 });
+
+describe('TrackCustomMarker with committed range scaling', function () {
+  autoMockCanvasContext();
+  autoMockElementSize({ width: GRAPH_WIDTH, height: GRAPH_HEIGHT });
+  autoMockIntersectionObserver();
+  beforeEach(addRootOverlayElement);
+  afterEach(removeRootOverlayElement);
+
+  it('uses the committed range scaling selector correctly', function () {
+    // Create markers with values: [1, 100, 2, 3] at times 0, 1, 2, 3
+    // Full range min/max would be 1-100, but committed range 2-3 should be 2-3
+    const { profile, dispatch, getState, markerStringIndex } = setup([
+      1, 100, 2, 3,
+    ]);
+
+    // Get the selectors to test them directly
+    const state = getState();
+    const threadSelectors = getThreadSelectors(0);
+    const markerSchema = ensureExists(
+      profile.meta.markerSchema.find((schema) => schema.name === 'Marker')
+    );
+    const markerTrackSelectors = threadSelectors.getMarkerTrackSelectors(
+      markerSchema,
+      markerStringIndex
+    );
+
+    // Get initial samples (full range) - should have min=1, max=200 (because second line = first * 2)
+    const fullRangeSamples =
+      markerTrackSelectors.getCollectedCustomMarkerSamples(state);
+    const fullRangeValueBounds =
+      markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(state);
+    expect(fullRangeValueBounds.minNumber).toBe(1);
+    expect(fullRangeValueBounds.maxNumber).toBe(200); // 100 * 2 from second line
+
+    // Commit range from 2ms to 3.5ms to include only the markers at times 2ms and 3ms.
+    // The 2.1 value is to exclude from the range the end of the marker at 1ms.
+    act(() => {
+      dispatch(commitRange(2.1, 3.5));
+    });
+
+    // Get committed range samples - should include markers at 2ms[2,4] and 3ms[3,6]
+    const newState = getState();
+    const committedRangeSamples =
+      markerTrackSelectors.getCollectedCustomMarkerSamples(newState);
+    const committedRangeValueBounds =
+      markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(newState);
+    expect(committedRangeValueBounds.minNumber).toBe(2);
+    expect(committedRangeValueBounds.maxNumber).toBe(6);
+
+    // Verify the arrays are the same length (same structure)
+    expect(committedRangeSamples.numbersPerLine).toHaveLength(
+      fullRangeSamples.numbersPerLine.length
+    );
+    expect(committedRangeSamples.markerIndexes).toHaveLength(
+      fullRangeSamples.markerIndexes.length
+    );
+  });
+
+  it('handles edge case where committed range has identical values', function () {
+    // Create markers with identical values in the committed range
+    const { profile, dispatch, getState, markerStringIndex } = setup([
+      1, 100, 5, 5,
+    ]);
+
+    const threadSelectors = getThreadSelectors(0);
+    const markerSchema = ensureExists(
+      profile.meta.markerSchema.find((schema) => schema.name === 'Marker')
+    );
+    const markerTrackSelectors = threadSelectors.getMarkerTrackSelectors(
+      markerSchema,
+      markerStringIndex
+    );
+
+    // Focus on the range with identical values at times 2ms and 3ms (values [5,10] and [5,10])
+    act(() => {
+      dispatch(commitRange(2.1, 3.5));
+    });
+
+    // Should not crash when min === max - both markers have first=5, second=10
+    const state = getState();
+    const committedRangeValueBounds =
+      markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(state);
+    expect(committedRangeValueBounds.minNumber).toBe(5);
+    expect(committedRangeValueBounds.maxNumber).toBe(10); // 5 * 2 from second line
+  });
+});
diff --git a/src/types/profile-derived.ts b/src/types/profile-derived.ts
@@ -584,14 +584,17 @@ export type AccumulatedCounterSamples = {
  * A collection of the data for all configured lines for a given marker
  */
 export type CollectedCustomMarkerSamples = {
-  readonly minNumber: number;
-  readonly maxNumber: number;
   // This value holds the number per configured line
   // selection. The array will share the indexes of the range filtered marker samples.
   readonly numbersPerLine: number[][];
   readonly markerIndexes: MarkerIndex[];
 };
 
+export type ValueBounds = {
+  readonly minNumber: number;
+  readonly maxNumber: number;
+};
+
 export type StackType = 'js' | 'native' | 'unsymbolicated';
 
 export type GlobalTrack =
