[profiler] Mark the node the metrics are about, and trace its edges - #6949
Karakatiza666 wants to merge 1 commit into
Conversation
| if (this.stickyInformation) { | ||
| if (!this.reportIsMarked()) { | ||
| this.traceSelection(node); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The commit message's central invariant — "the report and the glow cannot disagree about which node holds it" — doesn't hold, and the manual test plan describes behaviour this code can't produce.
traceSelection always calls markSelected, so the trace and the glow are inseparable; "hovering elsewhere moves the trace but leaves the glow on the pinned node" is not reachable.
Two concrete paths:
| steps | report panel | glow |
|---|---|---|
| click node A, hover node B | A | A (hover is a no-op — the trace does not move, contrary to the description) |
| click an expanded region R, hover operator B | R | B |
The second case is the disagreement: reportOn deliberately lets an expanded region report, traceSelection refuses to mark it, so reportIsMarked() is false and the next hover paints the glow on a node whose metrics are not on display. Either the sticky-hover branch should not mark (only colour edges, as the description says), or a region should not be reportable.
| return false; | ||
| } | ||
| this.center(Option.some(value)); | ||
| this.markSelected(el.nodes()); |
There was a problem hiding this comment.
search() marks but never reports, so the glow lands on the searched node while the panel still shows the previously clicked one — the same report/glow disagreement as in hoverNode, and it makes markSelected's doc ("the one the diagram reports on ... whether reached by click, hover or search") untrue. It also skips the edge trace, so a searched node glows without its edges coloured, unlike a clicked one.
Three edge cases this path gets wrong, none covered by a test:
getElementById(value)can resolve an edge:nonempty()is true,el.nodes()is empty, somarkSelectedclears the existing mark and adds none, yetsearchreturnstrue. Verified against a real headless cytoscape instance.- searching an expanded region or the root:
markSelectedadds the class,nodeShadow()refuses to paint a parent, so the previous glow is silently dropped and nothing replaces it.traceSelection's guards are bypassed here. - searching a node inside a collapsed region:
search()returns false,profiler.tsreveal()expands ancestors and centres after the layout, and the node is never marked at all.
Routing this through traceSelection (or displayNodeAttributes) rather than raw markSelected would give all four entry points one set of rules.
| // The body box cytoscape draws, which is the node's own size plus its padding. The | ||
| // border straddles the edge of that box and so adds nothing to it. | ||
| const padding = Number(node.numericStyle('padding')) || 0; | ||
| paintShadow( | ||
| context, | ||
| node, | ||
| shadow, | ||
| pos ?? node.position(), | ||
| w ?? node.width() + 2 * padding, | ||
| h ?? node.height() + 2 * padding |
There was a problem hiding this comment.
Worth knowing that this fallback is the only path that runs in a browser, not a defensive branch: cytoscape's cached-element pass calls drawElementUnderlay(context, ele) with two arguments (cytoscape.cjs.js, CRp$a.drawElementUnderlay), and the five-argument call from drawNode is guarded by shouldDrawOverlay, which the texture-cache draw passes as false. So pos/w/h are always undefined on screen, and the test that pins the five-argument shape exercises a call site the renderer never takes.
Two consequences:
node.padding()is cytoscape's own resolver for this and is whatdrawNodeOverlayUnderlayuses for the identical fallback;numericStyle('padding')returns the bare number and would be wrong for a percentage padding. Both current paddings are px, so no bug today — but reusingnode.padding()costs nothing and can't drift.drawCachedElementskips a node whose bounding box misses the viewport extent, and the box doesn't include the 20-unit glow, so a marked node just off-screen loses its glow entirely instead of bleeding in.
A test driving a real (non-headless) cytoscape renderer, or the browser harness later in the stack, would have caught the argument-shape assumption.
|
Ran in
No flakiness, no dependency, licensing, |
998ce8c to
f2431c0
Compare
f2431c0 to
7d153e8
Compare
d520626 to
60f2c05
Compare
60f2c05 to
0f6ad88
Compare
0f6ad88 to
eea5e1d
Compare
eea5e1d to
0c366f6
Compare
0c366f6 to
d20121d
Compare
Which node the metrics panel is describing was readable only from the
panel itself. The node now carries an accent glow, painted by
`nodeShadow.ts` in place of the ambient shadow every node has, and one
node holds the mark at a time - reached by click, by hover or by search.
Clicks and hovers went through one `displayEventTargetAttributes` with an
`isSticky` flag, which read as one behaviour and was two. They are now
`reportOn` and `hoverNode`:
click reports, and the report stays; an expanded region reports as
readily as an operator
hover reports only while nothing is pinned; a pinned report keeps its
mark, and a hover over anything else moves the trace alone
The mark is asked of cytoscape rather than tracked beside it, so the
report and the glow cannot disagree about which node holds it.
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
d20121d to
9488890
Compare
Part of #6895, split one commit per PR.
Which node the metrics panel is describing was readable only from the
panel itself. The node now carries an accent glow, painted by
nodeShadow.tsin place of the ambient shadow every node has, and onenode holds the mark at a time - reached by click, by hover or by search.
Clicks and hovers went through one
displayEventTargetAttributeswith anisStickyflag, which read as one behaviour and was two. They are nowreportOnandhoverNode:click reports, and the report stays; an expanded region reports as
readily as an operator
hover reports only while nothing is pinned; a pinned report keeps its
mark, and a hover over anything else moves the trace alone
The mark is asked of cytoscape rather than tracked beside it, so the
report and the glow cannot disagree about which node holds it.
Describe Manual Test Plan
Click a node: it takes an accent glow and its edges are traced. Hovering elsewhere moves the trace but leaves the glow on the pinned node.
Verified at this commit, not just at the tip of the stack: checked out detached with
js-packages/profiler-lib/distdeleted and rebuilt from this commit's source, thenprofiler-libbun run checkandbun run test, andprofiler-layoutbun run checkandbun run test(all three vitest projects, browser suites included). All four green.Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes
None. The change is confined to
js-packages/.