-
Notifications
You must be signed in to change notification settings - Fork 3k
Added editing option to nodes #502
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: main
Are you sure you want to change the base?
Conversation
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
Adds an edit capability to view and modify node content, including UI updates and supporting store logic, plus initial Cypress setup and tests.
- Introduces in-modal edit mode with Save/Cancel to update JSON via path resolution.
- Extends graph store with setSelectedNodeText to refresh node text after save.
- Adds Cypress config and an e2e test; minor UI updates in Tree/Graph views and sample JSON changes.
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/store/useFile.ts | Updates the default JSON sample content to a simpler object-based example. |
| src/features/modals/NodeModal/index.tsx | Adds edit mode (Textarea), save/cancel logic, path handling, and updates store with changes. |
| src/features/editor/views/TreeView/Value.tsx | Adds Popover with an Edit action in Tree view. |
| src/features/editor/views/GraphView/stores/useGraph.ts | Adds setSelectedNodeText to update node text in state. |
| src/features/editor/views/GraphView/CustomNode/index.tsx | Wraps node content in a to aid testing. |
| src/features/editor/views/GraphView/CustomNode/TextNode.tsx | Minor layout change; introduces unused imports/state related to future editing. |
| package.json | Adds Cypress scripts and dev dependency. |
| cypress.config.ts | Adds Cypress config for E2E and component testing. |
| cypress/support/* | Adds Cypress support files and component mount helpers. |
| cypress/e2e/node_modal_edit_button.cy.js | Adds an E2E test to find nodes with editable modals and count Edit/Save/Cancel. |
| cypress/fixtures/example.json | Adds a sample fixture. |
Comments suppressed due to low confidence (1)
cypress/e2e/node_modal_edit_button.cy.js:1
- Avoid using hashed or generated class names to close the modal; they are unstable across builds. Use a semantic selector like cy.get('button[aria-label="Close"]').click() or a data-testid on the close button.
describe('NodeModal Edit Button Visibility', () => {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| React.useEffect(() => { | ||
| console.log("useEffect triggered - selectedNode changed:", selectedNode); | ||
| if (selectedNode?.text !== undefined) { | ||
| const textString = dataToString(selectedNode.text); |
Copilot
AI
Oct 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.
editedText is initialized with dataToString(...), which strips quotes from string values, producing invalid JSON. Parsing editedText with JSON.parse will fail for common cases (e.g., string properties). Initialize and reset editedText with JSON.stringify(selectedNode.text, null, 2) instead, and reserve dataToString only for read-only display.
| const textString = dataToString(selectedNode.text); | |
| const textString = JSON.stringify(selectedNode.text, null, 2); |
| import useGraph from "../../editor/views/GraphView/stores/useGraph"; | ||
| import useFile from "../../../store/useFile"; | ||
| import useJson from "../../../store/useJson"; | ||
|
|
Copilot
AI
Oct 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.
editedText is initialized with dataToString(...), which strips quotes from string values, producing invalid JSON. Parsing editedText with JSON.parse will fail for common cases (e.g., string properties). Initialize and reset editedText with JSON.stringify(selectedNode.text, null, 2) instead, and reserve dataToString only for read-only display.
|
|
||
| } catch (e) { | ||
| console.error("JSON parse error:", e); | ||
| alert("Invalid JSON format: "); |
Copilot
AI
Oct 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 error message lacks context and omits the actual parse error. Include the exception message so users know what to fix, e.g., alert(Invalid JSON: ${e instanceof Error ? e.message : String(e)}).
| alert("Invalid JSON format: "); | |
| alert(`Invalid JSON: ${e instanceof Error ? e.message : String(e)}`); |
| console.log("NodeModal render - selectedNode:", selectedNode); | ||
| console.log("NodeModal render - editMode:", editMode); | ||
| console.log("NodeModal render - opened:", opened); | ||
|
|
Copilot
AI
Oct 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.
Excessive console logging throughout the component will add noisy output and impact performance in production. Consider removing these logs or guard them under a development flag.
| import { Popover, Button } from "@mantine/core"; | ||
| import { useState } from "react"; |
Copilot
AI
Oct 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.
Popover, Button, and useState are imported but not used. Remove unused imports to avoid lint errors and keep the file clean.
| import { Popover, Button } from "@mantine/core"; | |
| import { useState } from "react"; |
| const Node = ({ node, x, y, hasCollapse = false }: CustomNodeProps) => { | ||
| const [popoverOpened, setPopoverOpened] = React.useState(false); | ||
| const [editing, setEditing] = useState(false); | ||
| const [editValue, setEditValue] = useState(node.text as string); |
Copilot
AI
Oct 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.
These state variables are declared but never used. Remove them until the edit UI is implemented in this component.
| const [editValue, setEditValue] = useState(node.text as string); |
| <span | ||
| style={{ | ||
| color: getValueColor({ | ||
| theme, | ||
| $value: valueAsString, | ||
| }), | ||
| cursor: "pointer", | ||
| }} | ||
| onClick={() => setOpened((o) => !o)} | ||
| > | ||
| <TextRenderer>{JSON.stringify(value)}</TextRenderer> | ||
| </span> |
Copilot
AI
Oct 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.
Clickable span lacks keyboard accessibility and proper semantics. Use a button (or add role='button' and onKeyDown handlers for Enter/Space) so keyboard users can activate it.
| <span | |
| style={{ | |
| color: getValueColor({ | |
| theme, | |
| $value: valueAsString, | |
| }), | |
| cursor: "pointer", | |
| }} | |
| onClick={() => setOpened((o) => !o)} | |
| > | |
| <TextRenderer>{JSON.stringify(value)}</TextRenderer> | |
| </span> | |
| <button | |
| type="button" | |
| style={{ | |
| color: getValueColor({ | |
| theme, | |
| $value: valueAsString, | |
| }), | |
| cursor: "pointer", | |
| background: "none", | |
| border: "none", | |
| padding: 0, | |
| font: "inherit", | |
| }} | |
| onClick={() => setOpened((o) => !o)} | |
| > | |
| <TextRenderer>{JSON.stringify(value)}</TextRenderer> | |
| </button> |
| cy.contains('button', 'Cancel').click(); | ||
| cy.get('.m_b5489c3c > .mantine-focus-auto').click(); | ||
| return false; // Stop after first editable node | ||
| } else { | ||
| cy.get('.m_b5489c3c > .mantine-focus-auto').click(); | ||
| } |
Copilot
AI
Oct 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.
Returning false inside a nested then() does not break out of cy.get(...).each(...). The loop will continue. Add a guard at the beginning of the each callback (e.g., if (editCount > 0) return false;) or restructure the test to find the first matching node before interacting.
| }); | ||
|
|
||
| it('finds nodes with editable modals and counts Edit, Save, Cancel buttons', () => { | ||
| cy.get('g[id^="ref-"][id*="node"] rect', { timeout: 5000 }).each(($rect, index, $list) => { |
Copilot
AI
Oct 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.
Selector relies on internal SVG id patterns and is brittle. Prefer the explicit test hook added in code (e.g., [data-testid='open-node-modal']) to make the test stable and intention-revealing.
| cy.get('g[id^="ref-"][id*="node"] rect', { timeout: 5000 }).each(($rect, index, $list) => { | |
| cy.get('[data-testid="node-rect"]', { timeout: 5000 }).each(($rect, index, $list) => { |
| const jsonPathToString = (path?: NodeData["path"]) => { | ||
| if (!path || path.length === 0) return "$"; | ||
| return `$["${path.join('"]["')}"]`; | ||
| // If after filtering we have no keys, replace the root |
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.
Better English.
| // If after filtering we have no keys, replace the root | |
| // If we have no keys after filtering, replace the root |
Added an "edit" button to each node when in graph view.