FE-502: Improve SDCPN file import/export with Zod validation and layout fix#8536
FE-502: Improve SDCPN file import/export with Zod validation and layout fix#8536kube wants to merge 4 commits intocf/petrinaut-copy-pastefrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview Reworks import to return structured results with errors, explicitly reject unknown Updates the editor UI to show an Written by Cursor Bugbot for commit c721144. This will update automatically on new commits. Configure here. |
🤖 Augment PR SummarySummary: This PR refactors Petrinaut’s SDCPN JSON import/export to be versioned and Zod-validated, and improves the import experience when node positions are missing. Changes:
Technical Notes: The new importer prefers the versioned schema first, falls back to the legacy schema, and uses user settings (compact vs classic node dimensions) when computing ELK layout. 🤖 Was this summary useful? React with 👍 or 👎 |
| const needsPosition = new Set<string>(); | ||
| if (options?.onlyMissingPositions) { | ||
| for (const place of sdcpn.places) { | ||
| if (place.x === 0 && place.y === 0) { |
There was a problem hiding this comment.
In onlyMissingPositions mode, a node is treated as “missing” only when both x and y are 0, but the import path can produce partially-missing coordinates (e.g. x filled to 0 while y was present), which would then never get a new position. This can leave imported nodes stuck at a 0 coordinate even though hadMissingPositions triggered layout.
Severity: medium
Other Locations
libs/@hashintel/petrinaut/src/file-format/import-sdcpn.ts:38libs/@hashintel/petrinaut/src/views/Editor/editor-view.tsx:147
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| // Fall back to legacy format | ||
| const legacy = legacySdcpnFileSchema.safeParse(data); |
There was a problem hiding this comment.
parseSDCPNFile currently requires the file to match the current SDCPN shape (colorId, differentialEquationId, etc.) before convertOldFormatToSDCPN runs, so pre-2025-11-28 exports (which use fields like place.type) will fail validation and never be migrated. If the goal is to keep old-file import working, this validation step may be too strict for legacy inputs.
Severity: medium
Other Locations
libs/@hashintel/petrinaut/src/file-format/types.ts:10
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| /** | ||
| * Schema for the legacy file format (no version/meta, just title + SDCPN data). | ||
| */ | ||
| export const legacySdcpnFileSchema = sdcpnSchema.extend({ |
There was a problem hiding this comment.
legacySdcpnFileSchema is non-strict, so a file that includes version/meta but fails sdcpnFileSchema (e.g., version > supported) can still be accepted as “legacy” after those keys are stripped. That could silently import future/incompatible versioned files instead of surfacing an error.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
82c0b1c to
5cb778f
Compare
7889b18 to
1a404fc
Compare
5cb778f to
9d9196d
Compare
| * @returns A promise that resolves to an array of node positions | ||
| * @param dims - Node dimensions for places and transitions | ||
| * @param options.onlyMissingPositions - When true, only nodes with x=0 and y=0 will receive new positions. | ||
| * Nodes that already have non-zero positions are included in the layout graph (so ELK can route around them) |
There was a problem hiding this comment.
The docstring says existing non-zero nodes are included “so ELK can route around them,” but elkNodes never passes existing x/y (or any fixed-position hint) into the ELK graph. In onlyMissingPositions mode this can yield returned positions that don’t account for the unchanged nodes, potentially causing overlaps when only the “missing” nodes get applied.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| const needsPosition = new Set<string>(); | ||
| if (options?.onlyMissingPositions) { | ||
| for (const place of sdcpn.places) { | ||
| if (place.x === 0 && place.y === 0) { |
There was a problem hiding this comment.
In onlyMissingPositions mode, treating (0,0) as “missing” can also catch nodes that were intentionally positioned at the origin (if some other node had missing coords and triggered relayout). That could unexpectedly move a legitimately-placed node.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| useSelectionCleanup(); | ||
|
|
||
| function handleNew() { | ||
| const handleCreateEmpty = useCallback(() => { |
There was a problem hiding this comment.
useCallback is used for handleCreateEmpty, but Petrinaut’s React Compiler guidance (CLAUDE.md) asks to avoid manual memoization unless there’s a specific incompatibility. If there isn’t a concrete need for stable identity here, this is extra complexity and may be worth reconsidering.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| /** | ||
| * Parses raw JSON data into an SDCPN, handling both versioned and legacy formats. | ||
| */ | ||
| export const parseSDCPNFile = (data: unknown): ImportResult => { |
There was a problem hiding this comment.
parseSDCPNFile is a key new parsing/validation surface, but I don’t see unit coverage for versioned vs legacy inputs or malformed JSON cases. Adding a small set of tests here would help lock in backward-compat and error-reporting behavior.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
- Move file format code into `src/file-format/` (export, import, remove-visual-info, old-formats) - Replace manual type guards with Zod schemas for import validation, matching clipboard types pattern - Add versioned file format envelope (version, meta.generator) to exports - Fix stale closure bug: run ELK layout before createNewNet so positions are baked into the data - Add `onlyMissingPositions` option to calculateGraphLayout for partial re-layout - Add ImportErrorDialog showing Zod validation errors with "Create empty net" fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Zod schema required iconSlug and displayColor on color types, but these fields are stripped by "export without visual info". Make them optional in the schema and fill defaults on import, matching how missing x/y positions are already handled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move old format Zod schema into old-formats/pre-2025-11-28/ - Integrate old format parsing into parseSDCPNFile so pre-2025-11-28 files are validated and converted during import (previously dead code) - Reject versioned files from legacy fallback path to prevent silently accepting unsupported future versions - Remove dead convertOldFormatToSDCPN call from editor-view - Remove unnecessary useCallback (React Compiler handles memoization) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move the unsupported-version rejection from a Zod .refine() (which never worked because z.object() strips unknown keys first) into parseSDCPNFile where we can inspect the raw data. Remove unused SDCPNFileFormat type export. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e6b7192 to
c721144
Compare
4353358 to
927d425
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| }, | ||
| title, | ||
| ...sdcpnToExport, | ||
| }; |
There was a problem hiding this comment.
Export spread order lets stale title overwrite current title
High Severity
The import produces SDCPNWithTitle objects where title is included in the SDCPN data. This title is passed as petriNetDefinition to createNewNet, persisting it in the stored net object. In exportSDCPN, ...sdcpnToExport is spread after the explicit title parameter, so the stale import-time title on petriNetDefinition overwrites the user's current title. If a user imports a file, renames the net, and exports, the exported file will contain the original import title instead of the renamed one.
Additional Locations (2)
| return {}; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
ELK layout ignores existing node positions for partial relayout
Low Severity
When onlyMissingPositions is set, the JSDoc states that nodes with existing positions are "included in the layout graph so ELK can route around them." However, the ELK nodes are built without x/y coordinates, so ELK has no knowledge of existing positions and lays out all nodes from scratch. For partial relayout scenarios (some nodes positioned, some not), ELK-assigned positions for missing nodes may overlap with existing ones. This doesn't affect the current import use case where all positions are missing.



🌟 What is the purpose of this PR?
Fixes the bug where importing an SDCPN file exported without visual info breaks the editor. Also improves the file format with Zod validation and a versioned envelope.
🔗 Related links
🔍 What does this change?
src/file-format/(export-sdcpn,import-sdcpn,remove-visual-info,old-formats/)clipboard/types.tsversion,meta.generator) to exported filesx/y, runs ELK layout before callingcreateNewNet(avoids a stale closure bug where positions were applied to the wrong net)onlyMissingPositionsoption tocalculateGraphLayoutfor partial re-layout of imported nodesImportErrorDialogshowing Zod validation errors with a "Create empty net" fallbackPre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
❓ How to test this?