Track metrics view explicitly so the segment indicator stays in sync#6463
Track metrics view explicitly so the segment indicator stays in sync#6463Karakatiza666 wants to merge 3 commits into
Conversation
|
|
||
| /** 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
| return typeof view === 'string' ? view : 'node' | ||
| } | ||
|
|
||
| /** Accepts `null` so "is there a node to restore?" is a single call. */ |
There was a problem hiding this comment.
this commend makes no sense in this context. What is "restore?"
| 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 |
There was a problem hiding this comment.
I don't see any state machine, maybe there is one in your head
| }) | ||
|
|
||
| // 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 |
There was a problem hiding this comment.
what does it mean "got stuck on?"
|
|
||
| // 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. |
mythical-fred
left a comment
There was a problem hiding this comment.
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.
c9eefcc to
c3b01ef
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
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 didmetricsMode = 'overview'inside thequeueMicrotaskaftershowGlobalMetrics(true). The new code only callsdiagram.showGlobalMetrics(true), leavinganalysisViewat 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)triggersdisplayNodeAttributeswithisSticky=true. In the sticky branch the new code doesif (isNodeView(analysisView)) lastNodeData = attrs— so if the previous view was{ node }, the overview payload silently overwriteslastNodeData, and the "Node" segment then has nothing useful to restore.- Fix: write
analysisView = 'overview'in thequeueMicrotaskcallback alongside theshowGlobalMetricscall.
-
nodeIdOfcouples profiler-layout to cytograph's title-string format (non-blocking, but worth a comment). It doesattrs.title.split(' ')[0], which assumes the title is<id> <operation>— set incytograph.tsvia the kv-loop that prependsidand appends" " + operation. That coupling is exactly the kind of brittle thing the rest of the PR is removing. Cheaper alternatives: cache the clickednodeIdfromonNodeClickinSupportBundleViewerLayout(you already have it), or thread the id throughNodeAttributesas a first-class field instead of splitting the display title. The latter is cleaner sinceNodeAttributesalready 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
left a comment
There was a problem hiding this comment.
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.
…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>
c3b01ef to
251499f
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
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.
Added
onNodeClickto 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