diff --git a/js-packages/profiler-layout/src/lib/components/BundleLogsView.svelte b/js-packages/profiler-layout/src/lib/components/BundleLogsView.svelte index e05cd5a9d70..b11440606b8 100644 --- a/js-packages/profiler-layout/src/lib/components/BundleLogsView.svelte +++ b/js-packages/profiler-layout/src/lib/components/BundleLogsView.svelte @@ -23,7 +23,9 @@ let search: SearchState = $state(emptySearchState) $effect(() => { - if (!lookup) return + if (!lookup) { + return + } return lookup.register(lookupTabId, (query) => { search = advanceSearch(search, query ? { kind: 'substring', query } : null) }) diff --git a/js-packages/profiler-layout/src/lib/components/MetricsView.svelte b/js-packages/profiler-layout/src/lib/components/MetricsView.svelte index 6bc37d584e5..07240db1fa2 100644 --- a/js-packages/profiler-layout/src/lib/components/MetricsView.svelte +++ b/js-packages/profiler-layout/src/lib/components/MetricsView.svelte @@ -3,16 +3,20 @@ export type MetricsMode = 'overview' | 'node' | 'top-nodes' - /** The synthetic root "region" node represents the whole circuit (the overview) rather than a - * single operator. Reused wherever we need to distinguish overview data from a single node. */ - export function isOverviewAttributes(nodeAttributes: NodeAttributes): boolean { - return nodeAttributes.title === 'n region' + /** The toplevel node represents the whole circuit (the overview) rather than a single operator. + * `rootNodeId` is the loaded profile's actual toplevel id; while it is `undefined` (no profile + * yet) nothing counts as the overview. */ + export function isOverviewAttributes( + nodeAttributes: NodeAttributes, + rootNodeId: string | undefined + ): boolean { + return rootNodeId !== undefined && nodeAttributes.nodeId === rootNodeId } - /** The node title is built as `${id} ${operation}`; the id (first token) is what `search()` - * matches against, so it's the query that links back to the node in the diagram. */ + /** The node id is what `search()` matches against, so it's the query that links back to the + * node in the diagram. */ export function nodeSearchQuery(nodeAttributes: NodeAttributes): string { - return nodeAttributes.title.split(' ')[0] ?? '' + return nodeAttributes.nodeId } @@ -25,6 +29,8 @@ interface Props { mode: MetricsMode tooltipData: TooltipData | null + /** The loaded profile's toplevel node id, used to recognise overview data. */ + rootNodeId: string | undefined /** When true, metrics flagged `advanced` in the profile metadata are included. */ showAdvanced: boolean /** Lookup coordinator; the view registers an imperative handler so each Enter on the @@ -35,13 +41,15 @@ onSearchNode?: (query: string) => void } - const { mode, tooltipData, showAdvanced, lookup, onSearchNode }: Props = $props() + const { mode, tooltipData, rootNodeId, showAdvanced, lookup, onSearchNode }: Props = $props() const nodeAttributes = $derived( tooltipData && 'nodeAttributes' in tooltipData ? tooltipData.nodeAttributes : null ) // Single-node data (a specific operator) as opposed to the whole-circuit overview. - const isNodeView = $derived(nodeAttributes ? !isOverviewAttributes(nodeAttributes) : false) + const isNodeView = $derived( + nodeAttributes ? !isOverviewAttributes(nodeAttributes, rootNodeId) : false + ) const blocks = $derived( nodeAttributes ? buildBlocks(nodeAttributes, showAdvanced) : [] ) diff --git a/js-packages/profiler-layout/src/lib/components/SupportBundleViewerLayout.svelte b/js-packages/profiler-layout/src/lib/components/SupportBundleViewerLayout.svelte index 8c883941da1..36cad6a3a31 100644 --- a/js-packages/profiler-layout/src/lib/components/SupportBundleViewerLayout.svelte +++ b/js-packages/profiler-layout/src/lib/components/SupportBundleViewerLayout.svelte @@ -14,6 +14,7 @@ Dataflow, JsonProfiles, MetricOption, + NodeAttributes, ProfilerCallbacks, SourcePositionRange, WorkerOption @@ -21,8 +22,9 @@ import { type Snippet, untrack } from 'svelte' import type { TriageResults } from 'triage-types' import { createLookupCoordinator } from '../functions/lookup' + import { type AnalysisView, isNodeView, metricsModeOf } from '../functions/metricsMode' import { severityLabel, uniqueCategories, uniqueSeverities } from '../functions/triage' - import { isOverviewAttributes, type MetricsMode } from './MetricsView.svelte' + import type { MetricsMode } from './MetricsView.svelte' import ProfilerDiagram from './ProfilerDiagram.svelte' import type { TooltipData } from './ProfilerTooltip.svelte' import ProfileTimestampSelector from './ProfileTimestampSelector.svelte' @@ -71,14 +73,20 @@ }: Props = $props() let profilerDiagram: ProfilerDiagram | undefined = $state() + // The loaded profile's toplevel node id, so the analysis panel can recognise overview data. + const diagramRootNodeId = $derived(profilerDiagram?.getProfile()?.rootNodeId) let tooltipData: TooltipData | null = $state(null) - let lastNodeData: TooltipData | null = $state(null) + // Most recently inspected operator; the "Node" segment restores it via `setMetricsMode('node')`. + let lastNodeData: NodeAttributes | null = $state(null) let metrics: MetricOption[] = $state([]) let selectedMetricId = $state('') let message = $state('') let error = $state('') let currentTab = $state('Metrics') - let metricsMode = $state('overview') + // Tracked explicitly so the SegmentedControl indicator follows the user's choice instead of a + // title heuristic on `tooltipData`. + let analysisView = $state('overview') + const metricsMode = $derived(metricsModeOf(analysisView)) let showAdvancedMetrics = $state(false) let issueSeverityFilter = $state<'all' | 'error' | 'warning' | 'info'>('all') let issueCategoryFilter = $state('all') @@ -111,28 +119,31 @@ const lookup = createLookupCoordinator() const callbacks: ProfilerCallbacks = { - // Profiler callbacks only update metrics data; they never drive view mode. The view - // (showGlobalMetrics / showTopNodes) is driven directly by the controls in setMetricsMode, - // so nothing here feeds back into a reactive effect. + // Sticky callbacks only refresh the payload — view state is driven by `onNodeClick` and + // `setMetricsMode`, never inferred from the data. displayNodeAttributes: (data, isSticky) => { if (!isSticky) { return } - const attrs = data.match({ some: (v) => ({ nodeAttributes: v }), none: () => null }) + const attrs = data.match({ some: (v) => v, none: () => null }) untrack(() => { if (attrs) { - const isOverview = isOverviewAttributes(attrs.nodeAttributes) currentTab = 'Metrics' - // A node click while viewing the top-nodes table should reveal that node's - // attributes. - metricsMode = isOverview ? 'overview' : 'node' - tooltipData = attrs - if (!isOverview) { + tooltipData = { nodeAttributes: attrs } + // Only cache while we're on a node view, so an `'overview'` round-trip can't + // overwrite the operator the user actually inspected. + if (isNodeView(analysisView)) { lastNodeData = attrs } } }) }, + onNodeClick: (nodeId) => { + // The only path outside `setMetricsMode` that switches to "Node"; `displayNodeAttributes` + // fires right after with the payload. + analysisView = { node: nodeId } + currentTab = 'Metrics' + }, displayTopNodes(data, _isSticky) { tooltipData = data.match({ some: (topNodes) => ({ @@ -185,9 +196,10 @@ profilerDiagram?.selectMetric(selectedMetricId) }) - // Initial display: show the overview once the diagram is ready and whenever a new profile - // loads. Depends only on profileData + profilerDiagram (NOT metricsMode), so it runs once - // per profile and never re-fires from the callbacks it triggers. + // Show the overview each time a profile loads. `analysisView` is reset first so the + // SegmentedControl indicator follows the new view, and so the sticky `displayNodeAttributes` + // that `showGlobalMetrics` triggers sees a non-node view and can't overwrite `lastNodeData` + // with the overview payload. $effect(() => { void profileData const diagram = profilerDiagram @@ -195,23 +207,24 @@ return } queueMicrotask(() => { + analysisView = 'overview' diagram.showGlobalMetrics(true) - metricsMode = 'overview' }) }) - /** Switch the metrics view. Driven directly by the SegmentedControl (a user action), so the - * profiler calls happen here rather than via an effect reacting to `metricsMode` — that - * decoupling is what removes the feedback loop. */ + /** Switch the analysis panel's view. Writes `analysisView` first so the SegmentedControl + * indicator follows the user's pick, then asks the visualizer for the matching payload. */ function setMetricsMode(mode: MetricsMode) { - metricsMode = mode currentTab = 'Metrics' if (mode === 'overview') { + analysisView = 'overview' profilerDiagram?.showGlobalMetrics(true) } else if (mode === 'top-nodes') { + analysisView = 'top-nodes' profilerDiagram?.showTopNodes(true) - } else if (mode === 'node') { - tooltipData = lastNodeData + } else if (mode === 'node' && lastNodeData) { + analysisView = { node: lastNodeData.nodeId } + tooltipData = { nodeAttributes: lastNodeData } } } @@ -247,6 +260,7 @@ const analysisTabProps = $derived({ metricsMode, tooltipData, + rootNodeId: diagramRootNodeId, showAdvancedMetrics, lookup, logText, diff --git a/js-packages/profiler-layout/src/lib/components/tabs/IssuesTab.svelte b/js-packages/profiler-layout/src/lib/components/tabs/IssuesTab.svelte index 1860c8ab3f0..545b596dc9e 100644 --- a/js-packages/profiler-layout/src/lib/components/tabs/IssuesTab.svelte +++ b/js-packages/profiler-layout/src/lib/components/tabs/IssuesTab.svelte @@ -2,7 +2,8 @@ import TriageResultsView from '../TriageResultsView.svelte' import type { AnalysisTabProps } from './MetricsTab.svelte' - let { triageResults, lookup, issueSeverityFilter, issueCategoryFilter }: AnalysisTabProps = $props() + let { triageResults, lookup, issueSeverityFilter, issueCategoryFilter }: AnalysisTabProps = + $props() { + it("returns 'overview' and 'top-nodes' unchanged", () => { + expect(metricsModeOf('overview')).toBe('overview') + expect(metricsModeOf('top-nodes')).toBe('top-nodes') + }) + + it("maps any `{ node }` to 'node' regardless of the operator id", () => { + expect(metricsModeOf({ node: '42' })).toBe('node') + expect(metricsModeOf({ node: 'persistent-id-xyz' })).toBe('node') + }) + + // Regression for the old title-based guess: the mode used to be inferred from + // `tooltipData.title === 'n region'`, so any profile whose root id wasn't `n` kept showing + // "Node" by mistake. Reading the mode straight from the `AnalysisView` removes that guesswork. + it('reads the mode from the `AnalysisView` for any root id', () => { + const seq: AnalysisView[] = ['overview', { node: 'root-42' }, 'top-nodes', 'overview'] + expect(seq.map(metricsModeOf)).toEqual(['overview', 'node', 'top-nodes', 'overview']) + }) +}) + +describe('isNodeView', () => { + it('narrows to `{ node }` and returns false for string variants', () => { + const v: AnalysisView = { node: 'op-7' } + if (isNodeView(v)) { + expect(v.node).toBe('op-7') + } else { + throw new Error('expected isNodeView to narrow') + } + expect(isNodeView('overview')).toBe(false) + expect(isNodeView('top-nodes')).toBe(false) + expect(isNodeView(null)).toBe(false) + }) +}) diff --git a/js-packages/profiler-layout/src/lib/functions/metricsMode.ts b/js-packages/profiler-layout/src/lib/functions/metricsMode.ts new file mode 100644 index 00000000000..68b0070ae75 --- /dev/null +++ b/js-packages/profiler-layout/src/lib/functions/metricsMode.ts @@ -0,0 +1,18 @@ +// Describes which view the analysis panel is showing, so the segmented control stays in sync +// with what the diagram displays. + +import type { MetricsMode } from '../components/MetricsView.svelte' + +/** Which view the analysis panel is currently showing: the whole-circuit overview, the ranked + * top nodes, or a single operator's metrics. When one operator is selected, its id is stored so the + * panel can return to that same operator later. */ +export type AnalysisView = 'overview' | 'top-nodes' | { node: string } + +export function metricsModeOf(view: AnalysisView): MetricsMode { + return typeof view === 'string' ? view : 'node' +} + +/** Accepts `null` so "is there a node to restore?" is a single call. */ +export function isNodeView(view: AnalysisView | null): view is { node: string } { + return view !== null && typeof view === 'object' +} diff --git a/js-packages/profiler-layout/src/lib/index.ts b/js-packages/profiler-layout/src/lib/index.ts index 4b77cbd2238..94d2bd71fdb 100644 --- a/js-packages/profiler-layout/src/lib/index.ts +++ b/js-packages/profiler-layout/src/lib/index.ts @@ -16,7 +16,7 @@ export { TriageResultsView } export type { ZipItem } from 'but-unzip' -export { createLookupCoordinator, type LookupCoordinator } from './functions/lookup' export { createLoadGuard } from './functions/loadGuard' +export { createLookupCoordinator, type LookupCoordinator } from './functions/lookup' export type { ProcessedProfile } from './functions/processZipBundle' export { getSuitableProfiles, processProfileFiles } from './functions/processZipBundle' diff --git a/js-packages/profiler-lib/src/cytograph.ts b/js-packages/profiler-lib/src/cytograph.ts index 6654901a7ca..5012ec285fa 100644 --- a/js-packages/profiler-lib/src/cytograph.ts +++ b/js-packages/profiler-lib/src/cytograph.ts @@ -884,6 +884,9 @@ export class CytographRendering { .on('click', 'node', (e) => { // Hide previous node information if any this.hideNodeInformation(); + // Fires before the attrs payload so consumers can switch view state without + // inferring it from data (distinguishes a click from a programmatic refresh). + this.callbacks.onNodeClick?.(e.target.id()); // Display current node this.displayEventTargetAttributes(e, true); }) @@ -1025,6 +1028,7 @@ export class CytographRendering { let visible = false; const tooltipData: NodeAttributes = { + nodeId, title: "", columns: [], rows: [], diff --git a/js-packages/profiler-lib/src/profile.test.ts b/js-packages/profiler-lib/src/profile.test.ts index 3782000f396..424586c90ba 100644 --- a/js-packages/profiler-lib/src/profile.test.ts +++ b/js-packages/profiler-lib/src/profile.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { BooleanValue, BytesValue, + CircuitProfile, CountValue, MissingValue, PercentValue, @@ -10,6 +11,22 @@ import { TimeValue } from './profile.js' +describe('CircuitProfile.isTop', () => { + it('recognises the toplevel node by the parsed root id', () => { + const profile = new CircuitProfile(1, 'n') + expect(profile.isTop('n')).toBe(true) + expect(profile.isTop('n0')).toBe(false) + expect(profile.isTop('42')).toBe(false) + }) + + it('does not assume the root id is "n"', () => { + // The format conventionally emits "n", but isTop must follow whatever the profile carries. + const profile = new CircuitProfile(1, 'root-7') + expect(profile.isTop('root-7')).toBe(true) + expect(profile.isTop('n')).toBe(false) + }) +}) + describe('CountValue', () => { it('toString formats whole numbers with K/M/B suffixes', () => { expect(new CountValue(0).toString()).toBe('0') diff --git a/js-packages/profiler-lib/src/profile.ts b/js-packages/profiler-lib/src/profile.ts index 4449d1ae51a..afa2850543e 100644 --- a/js-packages/profiler-lib/src/profile.ts +++ b/js-packages/profiler-lib/src/profile.ts @@ -1331,7 +1331,7 @@ export class CircuitProfile { * Profile graphs are always a single node containing everything else inside. * That node is pretty much ignored everywhere else in this code after parsing. */ isTop(node: NodeId): boolean { - return node === "n"; + return node === this.rootNodeId; } addNode(n: JsonSimpleNodeWrapper | JsonClusterWrapper, parent: Option) { @@ -1529,7 +1529,9 @@ export class CircuitProfile { return Option.some(ranges[0]!); } - constructor(readonly worker_count: number) { } + /** @param rootNodeId Id of the toplevel graph node, taken from the parsed profile rather than + * assumed, so {@link isTop} stays correct whatever id the format emits. */ + constructor(readonly worker_count: number, readonly rootNodeId: NodeId) { } // Scan the nodes and compute the range of each property computePropertyRanges() { @@ -1602,7 +1604,7 @@ export class CircuitProfile { // Decode the graph structure and create the nodes. // The graph itself is always a complex node. let rootNodeId = json.graph.nodes.id; - let result = new CircuitProfile(worker_count); + let result = new CircuitProfile(worker_count, rootNodeId); result.complexNodes.set(rootNodeId, new ComplexNode(rootNodeId, json.graph.nodes.label, worker_count)); for (const nodeWrapper of json.graph.nodes.nodes) { diff --git a/js-packages/profiler-lib/src/profiler.ts b/js-packages/profiler-lib/src/profiler.ts index 059fd142325..1159bdf6e92 100644 --- a/js-packages/profiler-lib/src/profiler.ts +++ b/js-packages/profiler-lib/src/profiler.ts @@ -40,6 +40,9 @@ export interface TooltipRow { /** Tooltip data structure */ export interface NodeAttributes { + /** The operator's graph node id, carried as a first-class field so consumers need not + * parse it back out of `title`. */ + nodeId: string; title: string; /** Column headers */ columns: string[]; @@ -77,6 +80,10 @@ export interface ProfilerCallbacks { * container is hidden during this window); `rendering = false` follows the `layoutstop` * event when the graph is on screen and interactive. */ onRenderingChange?: (rendering: boolean) => void; + + /** User-initiated single-click on a node (not fired by `showGlobalMetrics` / `showTopNodes`). + * Fires before the matching `displayNodeAttributes` so consumers can switch view state. */ + onNodeClick?: (nodeId: string) => void; } export interface VisualizerConfig {