[profiler] Make the corner chips pressable - #6952
Conversation
| let pressed: ChipHit | null = null; | ||
| // Both bound on the container in the capture phase, ahead of cytoscape's own press handling, which | ||
| // is bound on that same container in the bubble phase. | ||
| container.addEventListener('mousedown', (event) => { |
There was a problem hiding this comment.
These two listeners are added to the container and never removed, and installChipButtons returns no teardown, so CytographRendering.dispose() cannot drop them. ProfilerDiagram.svelte disposes and recreates a Visualizer on the same graphContainer whenever the profile data changes, so after one remount the container carries two pairs of handlers, and the older pair holds a destroyed cy — cytoscape.destroyRenderer() sets _private.renderer = null, so toGraphPoint dereferences null.
Reproduced against this branch (temporary test, real headless cy, renderer() stubbed to null exactly as destroyRenderer leaves it):
TypeError: Cannot read properties of null (reading 'projectIntoViewport')
❯ toGraphPoint src/chipButtons.ts:170:18
❯ chipAtMouse src/chipButtons.ts:233:23
❯ mousedown src/chipButtons.ts:241:53
So every mousedown in the diagram throws after the first profile reload, and the live handlers still hit-test against the stale instance. Suggest returning a disposer (or binding an AbortController signal) and calling it from dispose(), plus a test that a torn-down instance no longer reacts.
| // Touch and pen only: cytoscape routes those through its own touch handling and reports a tap. A | ||
| // mouse press never reaches here, being stopped below before cytoscape sees it. | ||
| cy.on('tap', (event: EventObject) => { | ||
| const hit = hitTestChips(cy, event.position.x, event.position.y); | ||
| if (hit !== null) { | ||
| dispatch(hit); | ||
| } | ||
| }); |
There was a problem hiding this comment.
"Touch and pen only" doesn't hold for a press that starts off a chip. Cytoscape emits tap from its mouseup path too (triggerEvents(down, ["click", "tap", "vclick"], e, {x: pos[0], y: pos[1]}), gated only on !didDrag && !dragged && !selecting && !isOverThresholdDrag, desktopTapThreshold = 4px), with the position of the release. The mousedown handler below only calls stopPropagation() when the press landed on a chip; otherwise cytoscape sets hoverData.capture and the tap fires normally.
So: press on the canvas 1–3px above a code chip (or on a region's title row just under its counter), release on the chip → this handler dispatches. That defeats the cancel rule the mouseup handler implements and that 'cancels a press let go of anywhere but the chip it started on' tests. Consider gating on the pointer type (e.g. only dispatch here when the tap came from touch/pen), or tracking a "cytoscape saw the press" flag.
| }, | ||
| // The code chip asks for the same source lookup a double click on an operator does, so | ||
| // consumers see one callback either way. | ||
| onShowSource: (node) => this.config.callbacks.onNodeDoubleClick?.(node, 'leaf') |
There was a problem hiding this comment.
The code chip is drawn on any node with has_source, composites included, so this reports 'leaf' for a group node. Both consumers (ProfilerLayout.handleNodeDoubleClick, SupportBundleViewerLayout) only branch on type !== 'leaf', so it works today, but the second argument is documented as the node's kind and this makes it not one. A 'source' type — or narrowing the callback to onShowSource at the public boundary — would keep it honest.
|
Ran locally at this commit:
Uncovered cases I derived that the suite does not exercise: teardown (nothing asserts the instance stops reacting after |
The two corner chips are the diagram's controls: the code chip opens the SQL the node came from, the counter expands or collapses the region it reports on. Cytoscape knows nothing about them - they are background images - so `chipButtons.ts` computes each chip's box from the resolved style and hit-tests presses against it, which is also what gives the cursor and the hover highlight. Both actions land where a pointer already went: a code chip press reports on its node and then asks for its source, the same lookup a double click on an operator does, so a consumer sees one callback either way. A counter press is the double click on the region. Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
c8b08aa to
c52ff18
Compare
Part 12 of 15 of #6895, split one commit per PR. Based on
redesign-profiler-diagram-11; merge in order.The stack (this is 12 of 15)
The two corner chips are the diagram's controls: the code chip opens the
SQL the node came from, the counter expands or collapses the region it
reports on. Cytoscape knows nothing about them - they are background
images - so
chipButtons.tscomputes each chip's box from the resolvedstyle and hit-tests presses against it, which is also what gives the
cursor and the hover highlight.
Both actions land where a pointer already went: a code chip press reports
on its node and then asks for its source, the same lookup a double click
on an operator does, so a consumer sees one callback either way. A counter
press is the double click on the region.
Describe Manual Test Plan
Press a node's code chip to open the SQL it came from; press a region's counter chip to expand or collapse it. The cursor turns over both, and they highlight on hover.
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/.