-
Notifications
You must be signed in to change notification settings - Fork 19
Add per-label node style customization (color, size, caption) #1303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
…lection Co-authored-by: gkorland <[email protected]>
Co-authored-by: gkorland <[email protected]>
…dd constants Co-authored-by: gkorland <[email protected]>
|
@copilot please fix the ci, tests are failing |
Co-authored-by: barakb <[email protected]>
Fixed the e2e test failures in commit 4ae5e2b. The tests were failing because they didn't wait for the customize style buttons to become visible after the graph info panel opened. Added proper async waits (10s for label buttons, 5s for customize buttons) instead of immediately checking visibility. CI is currently running to verify the fix. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements per-label node style customization, allowing users to customize visual appearance of nodes by selecting colors from a 16-color palette, adjusting node sizes (11 options from 0.5x to 2.6x), and choosing which property to display as the node caption. Customizations are persisted in localStorage per graph and label.
Key Changes
- Extended the Label interface with a LabelStyle type containing customColor, customSize, and customCaption properties
- Added CustomizeStylePanel component with color picker, size selector, and caption dropdown
- Integrated customization UI via palette icon buttons in the Graph Info panel
- Updated ForceGraph rendering to apply custom sizes and colors, with custom caption support
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
e2e/tests/customizeStyle.spec.ts |
New e2e test suite with 6 tests validating customize style button visibility, panel interactions, color/size/caption selection, and ESC key handling |
app/graph/graphInfo.tsx |
Added state management for customizing labels and integrated palette button with tooltip for each label; renders CustomizeStylePanel overlay when active |
app/graph/CustomizeStylePanel.tsx |
New component implementing the style customization UI with color, size, and caption selection; handles localStorage persistence and graph state updates |
app/components/ForceGraph.tsx |
Updated node rendering to apply custom sizes and colors; added custom caption logic to display selected properties; updated display name caching to use EMPTY_DISPLAY_NAME constant |
app/api/graph/model.ts |
Added EMPTY_DISPLAY_NAME constant, STYLE_COLORS palette, NODE_SIZE_OPTIONS array, LabelStyle interface; extended Label interface with style property; added loadLabelStyle method to Graph class for loading saved styles |
README.md |
Added documentation section describing the new node style customization feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if label has custom caption property | ||
| const customCaption = label.style?.customCaption; |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The custom caption logic retrieves the label again via getLabelWithFewestElements, which was already calculated earlier at line 467. This is redundant. Consider storing the label in a variable and reusing it here to avoid recalculating the same value.
| test(`@readwrite Validate customize style button is visible for each label`, async () => { | ||
| const graphName = getRandomString("graph"); | ||
| await apiCall.addGraph(graphName); | ||
| const graph = await browser.createNewPage(GraphInfoPage, urls.graphUrl); | ||
| await browser.setPageToFullScreen(); | ||
| await graph.selectGraphByName(graphName); | ||
| await graph.insertQuery(CREATE_QUERY); | ||
| await graph.clickRunQuery(); | ||
| await graph.openGraphInfoButton(); | ||
|
|
||
| // Wait for the label button to appear first | ||
| const labelButton = graph.page.getByTestId("graphInfoperson1Node"); | ||
| await labelButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
|
|
||
| // Check if customize style button exists for person1 label | ||
| const customizeButton = graph.page.getByTestId("customizeStyleperson1"); | ||
| await customizeButton.waitFor({ state: 'visible', timeout: 5000 }); | ||
| expect(await customizeButton.isVisible()).toBeTruthy(); | ||
|
|
||
| await apiCall.removeGraph(graphName); | ||
| }); | ||
|
|
||
| test(`@readwrite Validate clicking customize style button opens customization panel`, async () => { | ||
| const graphName = getRandomString("graph"); | ||
| await apiCall.addGraph(graphName); | ||
| const graph = await browser.createNewPage(GraphInfoPage, urls.graphUrl); | ||
| await browser.setPageToFullScreen(); | ||
| await graph.selectGraphByName(graphName); | ||
| await graph.insertQuery(CREATE_QUERY); | ||
| await graph.clickRunQuery(); | ||
| await graph.openGraphInfoButton(); | ||
|
|
||
| // Wait for the label button to appear first | ||
| const labelButton = graph.page.getByTestId("graphInfoperson1Node"); | ||
| await labelButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
|
|
||
| // Click customize style button | ||
| const customizeButton = graph.page.getByTestId("customizeStyleperson1"); | ||
| await customizeButton.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await customizeButton.click(); | ||
|
|
||
| // Verify customization panel is visible | ||
| const panelTitle = graph.page.getByText("Customize Style"); | ||
| await panelTitle.waitFor({ state: 'visible' }); | ||
| expect(await panelTitle.isVisible()).toBeTruthy(); | ||
|
|
||
| // Verify color section is visible | ||
| const colorSection = graph.page.getByText("Color:"); | ||
| expect(await colorSection.isVisible()).toBeTruthy(); | ||
|
|
||
| // Verify size section is visible | ||
| const sizeSection = graph.page.getByText("Size:"); | ||
| expect(await sizeSection.isVisible()).toBeTruthy(); | ||
|
|
||
| // Verify caption section is visible | ||
| const captionSection = graph.page.getByText("Caption:"); | ||
| expect(await captionSection.isVisible()).toBeTruthy(); | ||
|
|
||
| await apiCall.removeGraph(graphName); | ||
| }); | ||
|
|
||
| test(`@readwrite Validate color selection in customization panel`, async () => { | ||
| const graphName = getRandomString("graph"); | ||
| await apiCall.addGraph(graphName); | ||
| const graph = await browser.createNewPage(GraphInfoPage, urls.graphUrl); | ||
| await browser.setPageToFullScreen(); | ||
| await graph.selectGraphByName(graphName); | ||
| await graph.insertQuery(CREATE_QUERY); | ||
| await graph.clickRunQuery(); | ||
| await graph.openGraphInfoButton(); | ||
|
|
||
| // Wait for the label button to appear first | ||
| const labelButton = graph.page.getByTestId("graphInfoperson1Node"); | ||
| await labelButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
|
|
||
| // Click customize style button | ||
| const customizeButton = graph.page.getByTestId("customizeStyleperson1"); | ||
| await customizeButton.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await customizeButton.click(); | ||
|
|
||
| // Wait for panel and click a color option | ||
| const panelTitle = graph.page.getByText("Customize Style"); | ||
| await panelTitle.waitFor({ state: 'visible' }); | ||
| const colorButtons = graph.page.locator('button[aria-label^="Select color"]'); | ||
| await colorButtons.first().waitFor({ state: 'visible' }); | ||
| await colorButtons.first().click(); | ||
|
|
||
| // Close the panel | ||
| const closeButton = graph.page.getByTitle("Close"); | ||
| await closeButton.click(); | ||
|
|
||
| await apiCall.removeGraph(graphName); | ||
| }); | ||
|
|
||
| test(`@readwrite Validate size selection in customization panel`, async () => { | ||
| const graphName = getRandomString("graph"); | ||
| await apiCall.addGraph(graphName); | ||
| const graph = await browser.createNewPage(GraphInfoPage, urls.graphUrl); | ||
| await browser.setPageToFullScreen(); | ||
| await graph.selectGraphByName(graphName); | ||
| await graph.insertQuery(CREATE_QUERY); | ||
| await graph.clickRunQuery(); | ||
| await graph.openGraphInfoButton(); | ||
|
|
||
| // Wait for the label button to appear first | ||
| const labelButton = graph.page.getByTestId("graphInfoperson1Node"); | ||
| await labelButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
|
|
||
| // Click customize style button | ||
| const customizeButton = graph.page.getByTestId("customizeStyleperson1"); | ||
| await customizeButton.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await customizeButton.click(); | ||
|
|
||
| // Wait for panel and click a size option | ||
| const panelTitle = graph.page.getByText("Customize Style"); | ||
| await panelTitle.waitFor({ state: 'visible' }); | ||
| const sizeButtons = graph.page.locator('button[aria-label^="Select size"]'); | ||
| const secondSizeButton = sizeButtons.nth(1); | ||
| await secondSizeButton.waitFor({ state: 'visible' }); | ||
| await secondSizeButton.click(); | ||
|
|
||
| // Close the panel | ||
| const closeButton = graph.page.getByTitle("Close"); | ||
| await closeButton.click(); | ||
|
|
||
| await apiCall.removeGraph(graphName); | ||
| }); | ||
|
|
||
| test(`@readwrite Validate caption selection in customization panel`, async () => { | ||
| const graphName = getRandomString("graph"); | ||
| await apiCall.addGraph(graphName); | ||
| const graph = await browser.createNewPage(GraphInfoPage, urls.graphUrl); | ||
| await browser.setPageToFullScreen(); | ||
| await graph.selectGraphByName(graphName); | ||
| await graph.insertQuery(CREATE_QUERY); | ||
| await graph.clickRunQuery(); | ||
| await graph.openGraphInfoButton(); | ||
|
|
||
| // Wait for the label button to appear first | ||
| const labelButton = graph.page.getByTestId("graphInfoperson1Node"); | ||
| await labelButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
|
|
||
| // Click customize style button | ||
| const customizeButton = graph.page.getByTestId("customizeStyleperson1"); | ||
| await customizeButton.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await customizeButton.click(); | ||
|
|
||
| // Wait for panel and click a caption option | ||
| const panelTitle = graph.page.getByText("Customize Style"); | ||
| await panelTitle.waitFor({ state: 'visible' }); | ||
| const captionOption = graph.page.getByText("id").last(); | ||
| await captionOption.waitFor({ state: 'visible' }); | ||
| await captionOption.click(); | ||
|
|
||
| // Close the panel | ||
| const closeButton = graph.page.getByTitle("Close"); | ||
| await closeButton.click(); | ||
|
|
||
| await apiCall.removeGraph(graphName); | ||
| }); |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is significant code duplication across all test cases. The setup code (lines 28-35 or similar) and the pattern of waiting for label and customize buttons (lines 37-43 or similar) are repeated in every test. Consider extracting a helper function that handles the common setup and returns the necessary page objects and buttons to reduce duplication and improve maintainability.
| {Array.from(Labels.values()).filter(({ name }) => !!name).map((infoLabel) => { | ||
| // Get the full Label object from the graph | ||
| const label = graph.LabelsMap.get(infoLabel.name); | ||
| if (!label) return null; |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code retrieves a Label from graph.LabelsMap but uses infoLabel.name (from GraphInfo.Labels) as the key. If there's any mismatch in label names between these two sources, the lookup could fail and return null. While there's a null check, this could silently skip rendering the customize button for some labels. Consider adding a fallback or warning when a label from GraphInfo.Labels is not found in graph.LabelsMap.
| if (!label) return null; | |
| if (!label) { | |
| console.warn(`Label '${infoLabel.name}' from GraphInfo.Labels not found in graph.LabelsMap.`); | |
| return null; | |
| } |
app/graph/CustomizeStylePanel.tsx
Outdated
| ); | ||
|
|
||
| const saveStyleToStorage = useCallback((labelName: string, style: LabelStyle) => { | ||
| const storageKey = `labelStyle_${graph.Id}_${labelName}`; |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The storage key pattern uses graph.Id, but there's inconsistency in how localStorage keys handle special characters in label names. If a label name contains special characters like underscores or colons (which are valid in graph labels), the storage key could potentially conflict with or be ambiguous with other keys. Consider encoding the label name or using a delimiter that's guaranteed not to appear in graph IDs or label names.
| const storageKey = `labelStyle_${graph.Id}_${labelName}`; | |
| const storageKey = `labelStyle_${encodeURIComponent(graph.Id)}_${encodeURIComponent(labelName)}`; |
app/graph/CustomizeStylePanel.tsx
Outdated
| const handleColorSelect = (color: string) => { | ||
| setSelectedColor(color); | ||
|
|
||
| // Update label style | ||
| const updatedLabel = graph.LabelsMap.get(label.name); | ||
| if (updatedLabel) { | ||
| updatedLabel.style = { | ||
| ...updatedLabel.style, | ||
| customColor: color, | ||
| }; | ||
|
|
||
| // Update all nodes with this label | ||
| updatedLabel.elements.forEach(n => { | ||
| // eslint-disable-next-line no-param-reassign | ||
| n.color = color; | ||
| }); | ||
|
|
||
| // Persist to localStorage | ||
| saveStyleToStorage(label.name, { | ||
| ...updatedLabel.style, | ||
| customColor: color, | ||
| }); | ||
| } |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style updates directly mutate the graph.LabelsMap and node properties, but these mutations don't trigger React re-renders because they bypass React's state management. The ForceGraph component may not re-render to reflect the style changes immediately. Consider calling a state setter or triggering a graph update after modifying the label style to ensure the UI updates properly.
| data.nodes.forEach(node => { | ||
| node.displayName = ['', '']; | ||
| // eslint-disable-next-line no-param-reassign | ||
| node.displayName = [...EMPTY_DISPLAY_NAME]; |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spread operator is used to copy EMPTY_DISPLAY_NAME, but since it's a tuple of primitive strings, this creates a new array. However, the check on line 490 uses !line1 && !line2 which will be true even after this assignment since both elements are empty strings. This could cause unnecessary recalculation of display text on every render. Consider checking if displayName needs regeneration differently, or ensure the cached values are properly used.
| node.displayName = [...EMPTY_DISPLAY_NAME]; | |
| node.displayName = undefined; |
| // Get label style customization | ||
| const label = getLabelWithFewestElements(node.labels.map(l => graph.LabelsMap.get(l) || graph.createLabel([l])[0])); |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label is retrieved from the graph's LabelsMap on every render for each node. Since this data doesn't change during rendering, consider retrieving the label style information once before the rendering loop and storing it in a map keyed by label name. This would avoid repeated lookups in the LabelsMap for every node on every frame.
| // Get label style customization | |
| const label = getLabelWithFewestElements(node.labels.map(l => graph.LabelsMap.get(l) || graph.createLabel([l])[0])); | |
| // Get label style customization from cached labelStyleMap | |
| const labelObjs = node.labels.map(l => labelStyleMap[l] || graph.createLabel([l])[0]); | |
| const label = getLabelWithFewestElements(labelObjs); |
update canvas immediately and fix sidebar label colors
🔒 Trivy Security Scan Results |
…com/FalkorDB/falkordb-browser into copilot/add-color-palette-selection
Fix text contrast on dark backgrounds using WCAG calculation Implement explicit save workflow with real-time preview Add custom RGB color picker with inline hex input
Node Style Customization Feature ✅
This PR implements per-label node style customization allowing users to:
Recent Fix
Implementation Complete
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.