From ecfaae11f07d62d9869275ace193175bdb6db88c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20Qu=C3=A8ze?= Date: Fri, 29 May 2026 12:01:48 +0200 Subject: [PATCH] WIP: Honor colorField when the marker field has format 'unique-string' The marker chart's colorField lookup read the field value directly from the payload, but for fields with format 'unique-string' the payload holds a string-table index rather than the string itself, so the value never matched a valid GraphColor and the marker fell back to blue. Add a getMarkerFieldValue helper in marker-schema.ts that dereferences unique-string / flow-id / terminating-flow-id field values through the string table, use it from MarkerChart Canvas, and collapse two existing inline copies of the same pattern (markerPayloadMatchesSearch and getGroupingValue) onto the helper. The console.warn calls in markerPayloadMatchesSearch that fired only for malformed profiles are dropped along the way. --- src/components/marker-chart/Canvas.tsx | 24 +++---- src/profile-logic/marker-schema.ts | 70 +++++++++++++-------- src/profile-query/formatters/marker-info.ts | 25 +++----- src/test/components/MarkerChart.test.tsx | 61 +++++++++++++++++- 4 files changed, 125 insertions(+), 55 deletions(-) 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[] = [ [