[FEATURE] Add canvas panel plugin - #729
Conversation
39b9803 to
d73e5e2
Compare
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "kind": "Canvas", | |||
There was a problem hiding this comment.
It appears to me that this is a nodes panel rather than a canvas. Unless we are thinking about adding other things different from nodes in the future.
There was a problem hiding this comment.
Yes, We would like to develop it further with new things. I think adding background is one of the things that is already here and fits more into "canvas" than "nodes" plugin.
|
First of all, awesome job @adrianSepiol! Would be there an option to create a graph (nodes and connections) based on a query? for example network topology charts have queries that return a list of nodes and their connections. In that case could this panel create a graph from that information in addition to the manual placement of nodes? |
That is something we also discussed and would like to work on in the future, but the idea is to make this work with this spec for the first iteration and then add new functionalities gradually. |
| /> | ||
| </g> | ||
|
|
||
| {showLegend && ( |
There was a problem hiding this comment.
[NOTE] Rule: rendering-conditional-render — Use Ternaries Instead of &&
&& is used for conditional rendering (e.g. {showLegend && (<ThresholdLegend .../>)} at CanvasPanel.tsx:88, {bwd && <EdgeArrowMarker .../>} at EdgeLines.tsx:110). When the left operand is falsy but not null/undefined/false, it can render "0" or "". In this codebase it's safe since the values are booleans/objects, but ternaries are more explicit.
Fix: Replace with ternary, e.g. {showLegend ? <ThresholdLegend ... /> : null}.
(Also applies to canvas/src/components/shared/EdgeLines.tsx:110)
| export function PanelEdgeLayer({ spec, seriesByQueryIndex, k, paletteColors }: PanelEdgeLayerProps): ReactElement { | ||
| const nodes = spec.nodes ?? []; | ||
| const edges = spec.edges ?? []; | ||
| const nodeById = new Map(nodes.map((n) => [n.id, n])); |
There was a problem hiding this comment.
[NOTE] Rule: js-index-maps — Build Maps for Repeated Lookups
const nodeById = new Map(nodes.map((n) => [n.id, n])) is created on every render without useMemo. Since PanelEdgeLayer re-renders on zoom changes, this map is rebuilt every time even though spec.nodes hasn't changed.
Fix: Wrap in useMemo(() => new Map(nodes.map(...)), [nodes]).
| return { | ||
| palette: chartsTheme.thresholds.palette, | ||
| selection: muiTheme.palette.warning.main, | ||
| connection: muiTheme.palette.info.main, | ||
| snapHighlight: muiTheme.palette.success.main, | ||
| background: muiTheme.palette.background.paper, | ||
| divider: muiTheme.palette.divider, | ||
| text: muiTheme.palette.text.primary, | ||
| labelBackground: muiTheme.palette.background.paper, | ||
| labelBorder: muiTheme.palette.divider, | ||
| labelText: muiTheme.palette.text.primary, | ||
| nodeStroke: muiTheme.palette.background.paper, | ||
| nodeDefaultFill: muiTheme.palette.primary.main, | ||
| }; |
There was a problem hiding this comment.
[NOTE] Rule: rerender-derived-state-no-effect — Avoid Recreating Objects Each Render
useCanvasTheme (line 32) creates a new object literal on every call (lines 35-48). Since it's used in both EditorNode and EditorEdge (called every frame during drag), this causes unnecessary object allocation.
Fix: Memoize the return value: return useMemo(() => ({ palette: ..., ... }), [muiTheme, chartsTheme]).
| Color string `json:"color,omitempty" yaml:"color,omitempty"` | ||
| } |
| X2 *float64 `json:"x2,omitempty" yaml:"x2,omitempty"` | ||
| Y2 *float64 `json:"y2,omitempty" yaml:"y2,omitempty"` |
There was a problem hiding this comment.
Why do they have suffix '2'? Is this the edge's middle point?
| X float64 `json:"x" yaml:"x"` | ||
| Y float64 `json:"y" yaml:"y"` |
There was a problem hiding this comment.
It seems we are repeating 'X' and 'Y' everywhere. Could it have its own struct like Position or Coordinate or it would be too much? Not sure though. Forget it if it is an overkill. Your call.
| LabelPosition LabelPosition `json:"labelPosition,omitempty" yaml:"labelPosition,omitempty"` | ||
| LabelPadding float64 `json:"labelPadding,omitempty" yaml:"labelPadding,omitempty"` | ||
| Icon string `json:"icon,omitempty" yaml:"icon,omitempty"` | ||
| Link string `json:"link,omitempty" yaml:"link,omitempty"` |
There was a problem hiding this comment.
Wouldn't URL be a more appropriate name?
While a URL (Uniform Resource Locator) is the address of a resource on the web, a Link (or Hyperlink) is an element on the page that will take a user to another page.
https://www.geeksforgeeks.org/computer-networks/difference-between-url-and-link/
| <TextField | ||
| label="X" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(background.x)} | ||
| onChange={onIntFieldChange('x')} | ||
| sx={{ width: 80 }} | ||
| disabled={background.global} | ||
| /> | ||
| <TextField | ||
| label="Y" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(background.y)} | ||
| onChange={onIntFieldChange('y')} | ||
| sx={{ width: 80 }} | ||
| disabled={background.global} | ||
| /> | ||
| <TextField | ||
| label="Width" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(background.width)} | ||
| slotProps={{ htmlInput: { min: 1 } }} | ||
| onChange={onIntFieldChange('width', 1)} | ||
| sx={{ width: 80 }} | ||
| disabled={background.global} | ||
| /> | ||
| <TextField | ||
| label="Height" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(background.height)} | ||
| slotProps={{ htmlInput: { min: 1 } }} | ||
| onChange={onIntFieldChange('height', 1)} | ||
| sx={{ width: 80 }} | ||
| disabled={background.global} | ||
| /> |
There was a problem hiding this comment.
I have not tested this part yet to understand what it exactly does.
However, my very first impression is that the repetitive part could be shortened using a loop over an array of x, y, height, and width. It seems 90% of these Text fields are identical.
⚠️ I will come back to this part later during the test to understand what it does.
| function parseImageFit(value: string): BackgroundSpec['imageFit'] { | ||
| return IMAGE_FIT_OPTIONS.includes(value as BackgroundSpec['imageFit']) | ||
| ? (value as BackgroundSpec['imageFit']) | ||
| : undefined; | ||
| } |
There was a problem hiding this comment.
It seems this function guarantees that what is received complies with BackgroundSpec['imageFit'] otherwise it returns undefined.
Checking the rest of the PR I see it has been used in a Select with limited and expected options (Menu Items). So, the question would be why we need this, if it is always dealing with a set of deterministic values? The function would make sense if a free text input was also an option. Am I missing something?
<Select<BackgroundSpec['imageFit']>
label="Image fit"
value={background.imageFit ?? 'cover'}
onChange={(e) => onChange({ ...background, imageFit: parseImageFit(e.target.value ?? '') })}
MenuProps={{ PaperProps: { style: { maxHeight: 240 } } }}
>
<MenuItem value="cover">Cover</MenuItem>
<MenuItem value="contain">Contain</MenuItem>
<MenuItem value="stretch">Stretch</MenuItem>
</Select>|
@adrianSepiol
|
| <Select<BackgroundSpec['imageFit']> | ||
| label="Image fit" | ||
| value={background.imageFit ?? 'cover'} | ||
| onChange={(e) => onChange({ ...background, imageFit: parseImageFit(e.target.value ?? '') })} |
There was a problem hiding this comment.
I have already made a comment about the parseImageFit.
https://github.com/perses/plugins/pull/729/changes#r3804121986
| import { generateQueryNames, useDataQueriesContext } from '@perses-dev/plugin-system'; | ||
| import { AnchorPoint, EdgeSpec, NodeSpec } from '../../model'; | ||
|
|
||
| const ANCHOR_OPTIONS: AnchorPoint[] = ['n', 'ne', 'e', 'se', 's', 'sw', 'w', 'nw']; |
There was a problem hiding this comment.
It seems that you already have such an array called ANCHOR_KEYS which is an array of AnchorPoint.
Could it be reused here? If a specific ordering is required, perhaps we could define that ordering in one place and reuse it for both?
export const ANCHOR_OFFSETS: Record<AnchorPoint, [number, number]> = {
n: [0, -1],
s: [0, 1],
e: [1, 0],
w: [-1, 0],
nw: [-1, -1],
ne: [1, -1],
sw: [-1, 1],
se: [1, 1],
};
export const ANCHOR_KEYS = Object.keys(ANCHOR_OFFSETS) as AnchorPoint[];| onChange({ | ||
| ...edge, | ||
| target: e.target.value, | ||
| targetAnchor: edge.targetAnchor ?? 'n', |
There was a problem hiding this comment.
When targetAnchor is undefined, we always fall back to n.
Are multiple edges expected to be able to share the same anchor? If not, should we look up an available anchor instead?
Also, since the set of anchors is finite, how should we handle a node with more connected edges than available anchors?
|
|
||
| const onThicknessModeChange = useCallback( | ||
| (e: React.ChangeEvent<HTMLInputElement>): void => { | ||
| onChange({ ...edge, thicknessMode: e.target.value as 'fixed' | 'threshold' }); |
There was a problem hiding this comment.
Have you defined a type for 'fixed' | 'threshold'?
I think it would be better to cast to the type instead. WDYT?
| const v = e.target.value; | ||
| onChange({ ...edge, sourceQueryIndex: v === '' ? undefined : Number(v) }); | ||
| }, |
There was a problem hiding this comment.
Should we validate sourceQueryIndex here?
Currently any none-empty value is converted with Number(V), which could potentially result in NaN, a negative value, or non-integer index.
| const onTargetQueryIndexChange = useCallback( | ||
| (e: React.ChangeEvent<HTMLInputElement>): void => { | ||
| const v = e.target.value; | ||
| onChange({ ...edge, targetQueryIndex: v === '' ? undefined : Number(v) }); | ||
| }, | ||
| [edge, onChange] | ||
| ); |
There was a problem hiding this comment.
I think this function could be merged with onSourceQueryIndexChange.
So, my suggestion is to merge the two functions and add a property name to the params. (targetQueryIndex and sourceQueryIndex)
They are pursuing the same goal and duplication could be avoided.
| const onTargetLabelTemplateChange = useCallback( | ||
| (e: React.ChangeEvent<HTMLInputElement>): void => { | ||
| onChange({ ...edge, targetLabelTemplate: e.target.value || undefined }); | ||
| }, | ||
| [edge, onChange] | ||
| ); |
There was a problem hiding this comment.
This is a duplication of onSourceLabelTemplateChange. As already mentioned, a single function with a property name as an input could avoid duplication.
| <TextField | ||
| select | ||
| label="Source anchor" | ||
| size="small" | ||
| value={edge.sourceAnchor ?? 'n'} | ||
| onChange={onSourceAnchorChange} | ||
| slotProps={{ select: { MenuProps: { PaperProps: { style: { maxHeight: 240 } } } } }} | ||
| > | ||
| {ANCHOR_OPTIONS.map((a) => ( | ||
| <MenuItem key={a} value={a}> | ||
| {a} | ||
| </MenuItem> | ||
| ))} | ||
| </TextField> |
| }, [mode, spec, applyMove, applyResize, applyEdgeDrag]); | ||
| const displayNodes = useMemo(() => unsavedSpec.nodes ?? [], [unsavedSpec.nodes]); | ||
| const displayEdges = useMemo(() => unsavedSpec.edges ?? [], [unsavedSpec.edges]); | ||
| const nodeById = useMemo(() => new Map(displayNodes.map((n) => [n.id, n])), [displayNodes]); |
There was a problem hiding this comment.
Since this map contains multiple nodes keyed by ID, would nodesById be a clearer name?
| const selectedFloatingEdges = displayEdges.filter( | ||
| (ed): ed is FloatingEdge => selectedIds.has(ed.id) && isFloatingEdge(ed) | ||
| ); | ||
| return (mode.type === 'idle' || mode.type === 'resizing') && selectedNodes.length >= 1 |
There was a problem hiding this comment.
This is a subtle comment and I hope you forgive me 😄 ,
If you prefer your own syntax, please ignore this.
greater than 0 is just a little more idiomatic and reads naturally as there are some selected nodes than greater equal 1. (Which is also perfectly valid)
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [fitView, height, width]); |
There was a problem hiding this comment.
Why?
eslint-disable-next-line react-hooks/exhaustive-deps
displayEdges could change, no?
| case 'selecting': { | ||
| const ids = applySelection(); | ||
| selectItems(ids); | ||
| endInteraction(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
@shahrokni 👀 Test: Could it draw a selection box over an empty space of the canvas? Then what would happen?
| const onSvgDoubleClick = useCallback( | ||
| (event: MouseEvent<SVGSVGElement>): void => { | ||
| if (event.ctrlKey || event.metaKey) { | ||
| const boundingBox = nodeBoundingBox(displayNodes); | ||
| if (boundingBox) { | ||
| fitView(boundingBox, width, height); | ||
| } | ||
| } else { | ||
| resetPan(); | ||
| } | ||
| }, | ||
| [displayNodes, fitView, resetPan, width, height] | ||
| ); |
| const onKeyDown = useCallback( | ||
| (event: KeyboardEvent<SVGSVGElement>): void => { | ||
| if (event.key !== 'Delete' && event.key !== 'Backspace') { | ||
| return; | ||
| } | ||
| if (selectedIds.size > 0) { | ||
| deleteSelected(); | ||
| } | ||
| }, | ||
| [selectedIds, deleteSelected] | ||
| ); |
There was a problem hiding this comment.
Not with this PR, but as a follow up,
Probably we should consider a simple Undo, Redo support,
Imagine that you have selected many nodes in a huge canvas, and just unintentionally, you press Delete.
Would be painful to reorganize everything again.
| <EditorNode | ||
| key={node.id} | ||
| node={node} | ||
| isHovered={hoveredId === node.id} |
| // Copyright The Perses Authors | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); |
| const canvasWidth = containerRef.current?.clientWidth ?? 0; | ||
| const cx = transform.invertX(canvasWidth / 2); | ||
| const cy = transform.invertY(CANVAS_HEIGHT / 2); | ||
| addNode(cx, cy); |
There was a problem hiding this comment.
Should we avoid adding a node when the container is unavailable or has zero width?
Falling back to 0 here could result in calculating an unintended canvas position.
Simply, if no canvas or no width, why should we add a node?
| const canvasWidth = containerRef.current?.clientWidth ?? 0; | ||
| const k = transform.k > 0 ? transform.k : 1; | ||
| const width = canvasWidth > 0 ? canvasWidth / k : 200; | ||
| const height = CANVAS_HEIGHT > 0 ? CANVAS_HEIGHT / k : 150; | ||
| const x = transform.invertX(0); | ||
| const y = transform.invertY(0); | ||
| addBackground(x, y, width, height); |
There was a problem hiding this comment.
200 and 150 are not descriptive and are somehow random numbers.
I suggest we define named constants.
| const hasBackgrounds = (spec.backgrounds?.length ?? 0) > 0; | ||
| const hasNodes = (spec.nodes?.length ?? 0) > 0; | ||
| const hasEdges = (spec.edges?.length ?? 0) > 0; |
There was a problem hiding this comment.
This could be simplified, no?
Example,
const hasEdges = !!spec.edges?.length| <TextField | ||
| label="X" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(node.x)} | ||
| onChange={onIntFieldChange('x')} | ||
| sx={{ width: 80 }} | ||
| /> | ||
| <TextField | ||
| label="Y" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(node.y)} | ||
| onChange={onIntFieldChange('y')} | ||
| sx={{ width: 80 }} | ||
| /> | ||
| <TextField | ||
| label="Width" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(node.width)} | ||
| slotProps={{ htmlInput: { min: 8 } }} | ||
| onChange={onIntFieldChange('width', 8)} | ||
| sx={{ width: 80 }} | ||
| /> | ||
| <TextField | ||
| label="Height" | ||
| size="small" | ||
| type="number" | ||
| value={Math.round(node.height)} | ||
| slotProps={{ htmlInput: { min: 8 } }} | ||
| onChange={onIntFieldChange('height', 8)} | ||
| sx={{ width: 80 }} | ||
| /> |
There was a problem hiding this comment.
We have some duplication here. The Text Fields could be dynamically generated by a loop over and object-keys or array of X ,Y, Width, Height
| sx={{ width: 80 }} | ||
| /> | ||
| </Stack> | ||
|
|
|
|
||
| function resolveEdgeStyle( | ||
| queryIndex: number | undefined, | ||
| thicknessMode: 'fixed' | 'threshold' | undefined, |
There was a problem hiding this comment.
This has been used in different places. Maybe we should define a type?
| // Copyright The Perses Authors | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
@shahrokni Test 👀 Pre-ordered SVG layers. Could it be problamatic?
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // |
There was a problem hiding this comment.
@shahrokni Test 👀 Importance of Ctrl+ Click for select for larger canvases.
| x: number; | ||
| y: number; |
There was a problem hiding this comment.
Type Point as already mentioned?
| const rows: Array<{ color: string; label: string }> = [ | ||
| ...steps.map((step, i) => ({ | ||
| color: step.color ?? paletteColors[i] ?? defaultColor, | ||
| label: `≥ ${formatValue(step.value, format)}`, | ||
| })), | ||
| { color: defaultColor, label: 'default' }, | ||
| ].reverse(); |
There was a problem hiding this comment.
Question
Why should we reverse the array?
| } | ||
| } else if (existing >= 0) { | ||
| draft.edgeThresholdWidths.splice(existing, 1); | ||
| } |
There was a problem hiding this comment.
Syntax Preference (Could be ignored)
if (existing !== -1) OR
change the name to existingIdx
| @@ -0,0 +1,63 @@ | |||
| // Copyright The Perses Authors | |||
There was a problem hiding this comment.
@shahrokni Test 👀 Collision if it is a long list of legends?
| import React, { ReactElement } from 'react'; | ||
| import { midpoint } from '../../utils/edgeUtils'; | ||
|
|
||
| type Line = { x1: number; y1: number; x2: number; y2: number }; |
There was a problem hiding this comment.
Type Point as already mentioned?
Signed-off-by: Adrian Sepiół <a.sepiol@sap.com> [ENHANCEMENT] update panel schema re-exports to use @perses-dev/plugin-system panelEditorSchema and buildPanelEditorSchema have moved from @perses-dev/spec to @perses-dev/plugin-system. Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
0c35389 to
6e7d109
Compare
Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
994b9f4 to
5fee794
Compare
Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
| function onNodePropertiesChange(updated: NodeSpec): void { | ||
| onChange( | ||
| produce(spec, (draft) => { | ||
| const idx = (draft.nodes ?? []).findIndex((n) => n.id === updated.id); | ||
| if (idx !== -1 && draft.nodes) { | ||
| draft.nodes[idx] = updated; | ||
| } | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Readability Improvement Suggestion
const nodes = draft.nodes;
if (!nodes) {
return;
}
const idx = nodes.findIndex((n) => n.id === updated.id);
if (idx !== -1) {
nodes[idx] = updated
}| onChange( | ||
| produce(spec, (draft) => { | ||
| const idx = (draft.backgrounds ?? []).findIndex((bg) => bg.id === updated.id); | ||
| if (idx !== -1 && draft.backgrounds) { | ||
| draft.backgrounds[idx] = updated; | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
| x1: number; | ||
| y1: number; | ||
| x2: number; | ||
| y2: number; | ||
| snapTargetId?: string; | ||
| snapTargetAnchor?: AnchorPoint; | ||
| editingEdgeId?: string; | ||
| editingEnd?: 'source' | 'target'; |
There was a problem hiding this comment.
Are snapTargetId/snapTargetAnchor and editingEdgeId/editingEnd expected to always be present together?
If so, they should be grouped.
type SnapTarget = {
id: String;
anchor: AnchorPoint;
};
type EditingEdge = {
id: string;
end: EndEdge;
};
interface DragEdge {
snapTarget?: SnapTarget;
editingEdge?: EditingEdge;
}| anchor: AnchorPoint; | ||
| } | ||
|
|
||
| function reconnectTarget(edge: EdgeSpec, snap: SnapResult | null, pt: { x: number; y: number }): void { |
| } | ||
| } | ||
|
|
||
| function reconnectSource(edge: EdgeSpec, snap: SnapResult | null, pt: { x: number; y: number }): void { |
| } | ||
| } | ||
|
|
||
| function buildNewEdge(dragEdge: DragEdge, snap: SnapResult | null, pt: { x: number; y: number }): EdgeSpec { |
| beginEndpointDrag: ( | ||
| event: PointerEvent<SVGCircleElement>, | ||
| edgeId: string, | ||
| end: 'source' | 'target', |
|
|
||
| interface UseEdgeConnectResult { | ||
| dragEdge: DragEdge | null; | ||
| beginEdgeDrag: (nodeId: string, anchor: AnchorPoint, x: number, y: number) => void; |
| origNodes: Array<{ id: string; x: number; y: number }>; | ||
| origEdges: Array<{ id: string; x2: number; y2: number }>; |
| export interface SelectionRect { | ||
| x0: number; | ||
| y0: number; | ||
| x1: number; | ||
| y1: number; | ||
| } |
Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
4b84d3b to
7c250fb
Compare
…mponent Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>


Description
Related to: perses/perses#3845
Introduces the
canvaspanel plugin — a free-form network diagram editor for building weathermap-style dashboards. Users can place nodes, connect them with edges, and overlay background shapes or images. Node and edge colors can be driven by query-bound thresholds, and edge thickness can scale with metric values.Screenshots
Here's a walkthrough of the main interactions:
Example weather map created in canvas:
Adding a node:
Screen.Recording.2026-07-20.at.15.26.09.mov
After creating a node, you can specify its label and position, add a link, bind a query to it, and use the query value in the label or derive the color from thresholds.
Adding an edge:
Screen.Recording.2026-07-20.at.15.26.45.mov
Edges are created by dragging from one node to another. They can be bidirectional and, like nodes, can have a query bound to them.
Zoom, pan, and resize:
Screen.Recording.2026-07-20.at.15.32.35.mov
Adding backgrounds:
adding.background.mov
Multiple backgrounds can be added with an image or a solid color at varying opacity levels. If "Global" is selected, the background always fills the entire view regardless of pan/zoom; otherwise it is scoped to a specified area.
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes