Track metrics view explicitly so the segment indicator stays in sync by Karakatiza666 · Pull Request #6463 · feldera/feldera

Karakatiza666 · 2026-06-13T01:30:58Z

Added onNodeClick to porfiler-lib to directly track when the node is clicked to properly disambiguate overview - node stats - top nodes tab states. This fixes indicator not showing that you are viewing the "overview" metrics.

Testing: manual, added unit tests

mihaibudiu · 2026-06-13T01:39:51Z

+
+    /** 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;


normally the profiler should not know about user actions, it should provide some APIs to manipulate the profile. It is up to the UI to decide what happens on single click, not to the profiler.
But maybe it's too late, since the callbacks already have this kind of information.

It is up to the UI to decide what happens on single click

This is pretty much what we're doing here, though? Profiler-lib only communicates to the UI (profiler-layout) that the single or double click happened, and then the UI desscribes what happens as the result.
profiler-lib does not care what happens on the click, it only reports it (as well as manages the internal state)

yes, but probably the names of the functions should be different in the UI and in the profile. They should be named like user actions in the UI but like profile operations in the profiler.

mihaibudiu · 2026-06-13T01:41:52Z

+  return typeof view === 'string' ? view : 'node'
+}
+
+/** Accepts `null` so "is there a node to restore?" is a single call. */


this comment makes no sense in this context. What is "restore?"

Refer to the stored id described in the comment above. (which is currently missing)

mihaibudiu · 2026-06-13T01:42:21Z

+import type { NodeAttributes } from 'profiler-lib'
+import type { MetricsMode } from '../components/MetricsView.svelte'
+
+/** The analysis panel's current selection. `{ node }` carries the operator id so "Node" can be


I don't see any state machine, maybe there is one in your head

mihaibudiu · 2026-06-13T01:42:54Z

+  })
+
+  // Regression for the title-heuristic bug: previously the mode was derived from
+  // `tooltipData.title === 'n region'`, so any profile whose root id wasn't `n` got stuck on


what does it mean "got stuck on?"

mihaibudiu · 2026-06-13T01:43:06Z

+
+  // Regression for the title-heuristic bug: previously the mode was derived from
+  // `tooltipData.title === 'n region'`, so any profile whose root id wasn't `n` got stuck on
+  // "Node". With the explicit discriminator the projection is heuristic-free.


what is a projection?

mythical-fred

Good direction. Tracking analysisView explicitly instead of inferring view state from tooltip data removes a feedback-loop hazard and makes the segment indicator deterministic. The AnalysisView discriminated union ('overview' | 'top-nodes' | { node: string }) is a clean model.

Bonus points for adding a vitest config + unit tests for metricsMode.ts — exactly the kind of test infrastructure the profiler-layout package needed.

mythical-fred

Good direction — explicit AnalysisView state is much cleaner than the title-string heuristic, and pushing a dedicated onNodeClick callback into ProfilerCallbacks so cytograph can distinguish a user click from a programmatic refresh is the right shape. The unit tests for metricsModeOf / isNodeView plus the new vitest infrastructure for profiler-layout are a real win.

Two concerns, one of them real:

Profile-reload effect no longer resets analysisView (blocker-shaped, but soft because hard to hit). The old code did metricsMode = 'overview' inside the queueMicrotask after showGlobalMetrics(true). The new code only calls diagram.showGlobalMetrics(true), leaving analysisView at whatever it was for the previous profile. Two knock-on effects:
- SegmentedControl indicator stays on "Node" / "Top nodes" while the diagram is now showing the overview — exactly the desync this PR is fixing for clicks, recreated for the profile-load path.
- showGlobalMetrics(true) triggers displayNodeAttributes with isSticky=true. In the sticky branch the new code does if (isNodeView(analysisView)) lastNodeData = attrs — so if the previous view was { node }, the overview payload silently overwrites lastNodeData, and the "Node" segment then has nothing useful to restore.
- Fix: write analysisView = 'overview' in the queueMicrotask callback alongside the showGlobalMetrics call.
nodeIdOf couples profiler-layout to cytograph's title-string format (non-blocking, but worth a comment). It does attrs.title.split(' ')[0], which assumes the title is <id> <operation> — set in cytograph.ts via the kv-loop that prepends id and appends " " + operation. That coupling is exactly the kind of brittle thing the rest of the PR is removing. Cheaper alternatives: cache the clicked nodeId from onNodeClick in SupportBundleViewerLayout (you already have it), or thread the id through NodeAttributes as a first-class field instead of splitting the display title. The latter is cleaner since NodeAttributes already lives in profiler-lib.

Nit: minor diff noise in index.ts (reordered createLookupCoordinator / createLoadGuard) and IssuesTab.svelte (line wrapping the destructure) — fine, just don't squash them into the substantive commit message.

Once #1 is fixed, happy to re-approve.

mythical-fred

Rebased tip reviewed (c3b01efd73). The blocker from my prior review is still present:

The profile-reload $effect (around line 200 of SupportBundleViewerLayout.svelte) still only calls diagram.showGlobalMetrics(true) without resetting analysisView = 'overview' inside the queueMicrotask. Switching profiles while viewing a node will leave the SegmentedControl on "Node" while the diagram shows the overview — exactly the desync this PR fixes for clicks.

One-line fix: analysisView = 'overview' before diagram.showGlobalMetrics(true) in the microtask callback.

Keeping REQUEST_CHANGES from the prior review until this is addressed.

mythical-fred

Blocker resolved — the profile-reload $effect now sets analysisView = 'overview' inside the queueMicrotask right before showGlobalMetrics(true), so the SegmentedControl indicator follows the reload and the sticky displayNodeAttributes triggered by showGlobalMetrics finds a non-node view and leaves lastNodeData alone. My earlier non-blocking nit about nodeIdOf parsing the cytograph title is also gone — nodeId is a first-class field on NodeAttributes now, isOverviewAttributes compares against the loaded profile's actual rootNodeId, and CircuitProfile.isTop no longer hardcodes "n" (nice regression test for that).

LGTM.

mythical-fred

Re-APPROVE on 762ef22.

The two follow-ups since 251499f are exactly the right cleanup:

CircuitProfile now takes the parsed rootNodeId as a constructor argument; isTop compares against it instead of the hardcoded "n". NodeAttributes.nodeId is a first-class field, so MetricsView reads the id directly (nodeAttributes.nodeId) and isOverviewAttributes compares against rootNodeId rather than the "n region" title string. This kills the title-format coupling my earlier review flagged.
nodeSearchQuery drops the title.split(' ')[0] parsing and just returns nodeAttributes.nodeId — the right shape now that the id is on the type.
SupportBundleViewerLayout threads diagramRootNodeId from profilerDiagram.getProfile()?.rootNodeId down to MetricsTab, and the new regression test in profile.test.ts pins the "any root id" behavior (covers the case where the format emits something other than "n").
Small UI bonus: parent ID / persistent ID rendered next to the node title in the analysis panel, gated on isNodeView so the overview header stays clean.

Nothing blocking. Good follow-through.

mihaibudiu · 2026-06-16T23:19:21Z


-    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. */


the part after "rather than" is not useful.

mihaibudiu · 2026-06-16T23:20:32Z

+
+    /** 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;


yes, but probably the names of the functions should be different in the UI and in the profile. They should be named like user actions in the UI but like profile operations in the profiler.

mihaibudiu · 2026-06-16T23:21:03Z

+
+/** 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. */


can you say where the id is stored?

For clarity I'll rename { node: string } -> { nodeId: string } - that's where the id is stored

mihaibudiu · 2026-06-16T23:21:25Z

+  return typeof view === 'string' ? view : 'node'
+}
+
+/** Accepts `null` so "is there a node to restore?" is a single call. */


Refer to the stored id described in the comment above. (which is currently missing)

Karakatiza666 · 2026-06-17T16:59:04Z

They should be named like user actions in the UI but like profile operations in the profiler.

I don't agree. profiler-lib is not a pure algorithm lib, it implements graphical visuialization, so it's not completely decoupled from user presentation. So it has to represent user actions on that presentation; the other way would be to expose the low-level implementation of nodes so that the UI can register the onClick handler itself, but nothing warrants this indirection.

…stays in sync The analysis panel's view is an explicit discriminated union — `AnalysisView = 'overview' | 'top-nodes' | { node: string }` — projected to the SegmentedControl mode via `metricsModeOf`. Every transition (segment click, diagram click, initial mount) writes `analysisView` directly with the value the action chose, and a new `onNodeClick` callback on `ProfilerCallbacks` lets cytograph distinguish a user diagram click from a programmatic `showGlobalMetrics` / `showTopNodes` refresh. `displayNodeAttributes` only refreshes `tooltipData`. The "Node" segment restores the previous operator by building `{ node: nodeIdOf(lastNodeData) }` from the cached attributes; `lastNodeData` is narrowed to `NodeAttributes`.

…me as a constant Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>

… it's matches the hard-coded constant Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>

mythical-fred

LGTM — rootNodeId as a first-class ctor arg and the nodeId field on NodeAttributes cleanly resolve the title-coupling concern.

mihaibudiu reviewed Jun 13, 2026

View reviewed changes

mythical-fred reviewed Jun 14, 2026

View reviewed changes

Karakatiza666 force-pushed the issue-segment-indicator branch from c9eefcc to c3b01ef Compare June 14, 2026 11:32

Karakatiza666 marked this pull request as ready for review June 14, 2026 11:32

mythical-fred suggested changes Jun 14, 2026

View reviewed changes

mythical-fred reviewed Jun 14, 2026

View reviewed changes

Karakatiza666 force-pushed the issue-segment-indicator branch from c3b01ef to 251499f Compare June 15, 2026 11:10

mythical-fred approved these changes Jun 15, 2026

View reviewed changes

Karakatiza666 force-pushed the issue-segment-indicator branch from 251499f to 762ef22 Compare June 16, 2026 17:58

mythical-fred approved these changes Jun 16, 2026

View reviewed changes

mihaibudiu approved these changes Jun 16, 2026

View reviewed changes

Karakatiza666 added 2 commits June 17, 2026 17:33

[profiler-lib] Expose nodeId as a node attribute, export root node na…

749a1ad

…me as a constant Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>

Karakatiza666 force-pushed the issue-segment-indicator branch from 762ef22 to 8d5c623 Compare June 17, 2026 17:33

[profiler-lib] Determine root node id dynamically instead of assuming…

9a8edfd

… it's matches the hard-coded constant Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>

Karakatiza666 force-pushed the issue-segment-indicator branch from 8d5c623 to 9a8edfd Compare June 17, 2026 18:12

Karakatiza666 enabled auto-merge June 17, 2026 18:12

Karakatiza666 added this pull request to the merge queue Jun 17, 2026

mythical-fred approved these changes Jun 17, 2026

View reviewed changes

Merged via the queue into main with commit bf0f071 Jun 17, 2026
1 check passed

Karakatiza666 deleted the issue-segment-indicator branch June 17, 2026 20:18

Conversation

Karakatiza666 commented Jun 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karakatiza666 commented Jun 17, 2026

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mihaibudiu Jun 13, 2026 •

edited

Loading