diff --git a/src/components/marker-chart/Canvas.tsx b/src/components/marker-chart/Canvas.tsx index 491a81e5fb..3072813af4 100644 --- a/src/components/marker-chart/Canvas.tsx +++ b/src/components/marker-chart/Canvas.tsx @@ -58,7 +58,10 @@ import { getTextColor, isValidGraphColor, } from 'firefox-profiler/profile-logic/graph-color'; -import { getSchemaFromMarker } from 'firefox-profiler/profile-logic/marker-schema'; +import { + getSchemaFromMarker, + getMarkerFieldValue, +} from 'firefox-profiler/profile-logic/marker-schema'; import { getBottomBoxInfoForStackFrame } from 'firefox-profiler/profile-logic/bottom-box'; import type { @@ -203,7 +206,7 @@ class MarkerChartCanvasImpl extends React.PureComponent { strokeColor: string; textColor: string; } { - const { getMarker, markerSchemaByName } = this.props; + const { getMarker, markerSchemaByName, thread } = this.props; const marker = getMarker(markerIndex); let color: GraphColor | null = null; @@ -211,17 +214,16 @@ class MarkerChartCanvasImpl extends React.PureComponent { // Try to get color from the marker schema's colorField const schema = getSchemaFromMarker(markerSchemaByName, marker.data); - if ( - schema && - schema.colorField && - marker.data && - typeof marker.data === 'object' - ) { - // Use type assertion to safely access dynamic property - const fieldValue = (marker.data as any)[schema.colorField]; + if (schema && schema.colorField) { + const fieldValue = getMarkerFieldValue( + marker.data, + schema.colorField, + schema, + thread.stringTable + ); // Validate that the field value is a valid GraphColor if (typeof fieldValue === 'string' && isValidGraphColor(fieldValue)) { - color = fieldValue as GraphColor; + color = fieldValue; } } diff --git a/src/profile-logic/marker-schema.ts b/src/profile-logic/marker-schema.ts index 014452278a..ff2e61d8bb 100644 --- a/src/profile-logic/marker-schema.ts +++ b/src/profile-logic/marker-schema.ts @@ -98,6 +98,42 @@ export function getSchemaFromMarker( return schemaName ? (markerSchemaByName[schemaName] ?? null) : null; } +/** + * Returns the value of a marker payload field. For fields declared as + * `unique-string` / `flow-id` / `terminating-flow-id` in the schema, the + * string-table index is dereferenced. Other formats — and fields not declared + * in the schema, or callers passing no schema — return the raw payload value. + * Returns undefined if the field isn't present on the marker, or if a + * unique-string field's value isn't a valid index into the string table. + */ +export function getMarkerFieldValue( + markerData: MarkerPayload | null, + fieldKey: string, + markerSchema: MarkerSchema | null, + stringTable: StringTable +): unknown { + if (!markerData) { + return undefined; + } + const value = (markerData as any)[fieldKey]; + if (value === undefined || value === null) { + return undefined; + } + const field = markerSchema?.fields.find((f) => f.key === fieldKey); + if ( + field && + (field.format === 'unique-string' || + field.format === 'flow-id' || + field.format === 'terminating-flow-id') + ) { + if (typeof value !== 'number' || !stringTable.hasIndex(value)) { + return undefined; + } + return stringTable.getString(value); + } + return value; +} + // Matches ternary expressions inside marker labels, ie {marker.data.field ? 'truthy' : 'falsy'} const TERNARY_RE = /^\s*([\w.]+)\s*\?\s*'([^']*)'\s*:\s*'([^']*)'\s*$/; @@ -633,34 +669,16 @@ export function markerPayloadMatchesSearch( // Check if fields match the search regular expression. for (const payloadField of markerSchema.fields) { - let value = (data as any)[payloadField.key]; - if (value === undefined || value === null) { - // The value is missing, but this is OK, values are optional. + const value = getMarkerFieldValue( + data, + payloadField.key, + markerSchema, + stringTable + ); + if (value === undefined || value === '') { continue; } - - if ( - payloadField.format === 'unique-string' || - payloadField.format === 'flow-id' || - payloadField.format === 'terminating-flow-id' - ) { - if (typeof value !== 'number') { - console.warn( - `In marker ${marker.name}, the key ${payloadField.key} has an invalid value "${value}" as a unique string, it isn't a number.` - ); - continue; - } - - if (!stringTable.hasIndex(value)) { - console.warn( - `In marker ${marker.name}, the key ${payloadField.key} has an invalid index "${value}" as a unique string, as it's missing from the string table.` - ); - continue; - } - value = stringTable.getString(value); - } - - if (value !== '' && testFun(value, payloadField.key)) { + if (testFun(String(value), payloadField.key)) { return true; } } diff --git a/src/profile-query/formatters/marker-info.ts b/src/profile-query/formatters/marker-info.ts index e013493f79..7192da5b5a 100644 --- a/src/profile-query/formatters/marker-info.ts +++ b/src/profile-query/formatters/marker-info.ts @@ -13,6 +13,7 @@ import { getThreadSelectors } from 'firefox-profiler/selectors/per-thread'; import { formatFromMarkerSchema, getLabelGetter, + getMarkerFieldValue, } from 'firefox-profiler/profile-logic/marker-schema'; import { changeMarkersSearchString } from '../../actions/profile-view'; import type { Store } from '../../types/store'; @@ -307,24 +308,16 @@ function getGroupingValue( return categories[marker.category]?.name ?? 'Unknown'; } // Field-based grouping - const fieldValue = (marker.data as any)?.[key.field]; - if (fieldValue === undefined || fieldValue === null) { + const schema = marker.data ? markerSchemaByName[marker.data.type] : null; + const fieldValue = getMarkerFieldValue( + marker.data, + key.field, + schema, + stringTable + ); + if (fieldValue === undefined) { return '(no value)'; } - // For fields whose format stores a string-table index (unique-string / - // flow-id / terminating-flow-id), resolve to the interned string so groups - // show "Error" / "click" / ... instead of integer indices. - const schema = marker.data ? markerSchemaByName[marker.data.type] : undefined; - const field = schema?.fields.find((f) => f.key === key.field); - if ( - field && - (field.format === 'unique-string' || - field.format === 'flow-id' || - field.format === 'terminating-flow-id') && - typeof fieldValue === 'number' - ) { - return stringTable.getString(fieldValue, '(empty)'); - } return String(fieldValue); } diff --git a/src/test/components/MarkerChart.test.tsx b/src/test/components/MarkerChart.test.tsx index d789a8f5e6..139a804af0 100644 --- a/src/test/components/MarkerChart.test.tsx +++ b/src/test/components/MarkerChart.test.tsx @@ -15,6 +15,7 @@ import { fireEvent, act, } from 'firefox-profiler/test/fixtures/testing-library'; +import { StringTable } from '../../utils/string-table'; import { changeMarkersSearchString } from '../../actions/profile-view'; import { TIMELINE_MARGIN_LEFT, @@ -800,7 +801,10 @@ describe('MarkerChart', function () { }); describe('Colored markers', () => { - function setupColoredMarkerTest(markers: TestDefinedMarker[]) { + function setupColoredMarkerTest( + markers: TestDefinedMarker[], + colorFieldFormat: 'string' | 'unique-string' = 'string' + ) { const profile = getProfileWithMarkers(markers); // Add marker schema with colorField to the profile @@ -813,7 +817,7 @@ describe('MarkerChart', function () { { key: 'statusColor', label: 'Color', - format: 'string', + format: colorFieldFormat, hidden: true, }, ], @@ -821,6 +825,24 @@ describe('MarkerChart', function () { }, ]; + // getProfileWithMarkers only auto-interns unique-string fields for + // marker types in the default fixtures schema. The 'Test' schema is + // installed afterwards, so for unique-string we replace each + // statusColor value with its string-table index by hand. + if (colorFieldFormat === 'unique-string') { + const stringTable = StringTable.withBackingArray( + profile.shared.stringArray + ); + for (const thread of profile.threads) { + for (let i = 0; i < thread.markers.length; i++) { + const data: any = thread.markers.data[i]; + if (data && typeof data.statusColor === 'string') { + data.statusColor = stringTable.indexForString(data.statusColor); + } + } + } + } + const { flushRafCalls } = setupWithProfile(profile); flushRafCalls(); const drawLog = flushDrawLog(); @@ -879,6 +901,41 @@ describe('MarkerChart', function () { ); }); + it('renders markers with colors from a unique-string colorField', () => { + const markersWithColors: TestDefinedMarker[] = [ + [ + 'Green Test', + 0, + 5, + { + type: 'Test', + status: 'success', + statusColor: 'green', + }, + ], + [ + 'Red Test', + 6, + 10, + { + type: 'Test', + status: 'failure', + statusColor: 'red', + }, + ], + ]; + + const fillColors = setupColoredMarkerTest( + markersWithColors, + 'unique-string' + ); + + expect(fillColors).toEqual( + expect.arrayContaining([getFillColor('green'), getFillColor('red')]) + ); + expect(fillColors).not.toContain(DEFAULT_FILL_COLOR); + }); + it('falls back to default blue for markers without color data', () => { const markersWithoutColors: TestDefinedMarker[] = [ [