-
Notifications
You must be signed in to change notification settings - Fork 109
H-5840: SDCPN diagnostics panel with global type-checker #8171
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
Conversation
bb162e7 to
a1940ac
Compare
496cc7c to
79a4ea8
Compare
PR SummaryAdds a Diagnostics Panel powered by a TypeScript language service to validate SDCPN code, integrated into the editor UI and simulation flow with tests.
Written by Cursor Bugbot for commit 239cfae. This will update automatically on new commits. Configure here. |
| getCompilationSettings: () => COMPILER_OPTIONS, | ||
| getScriptVersion: () => "0", | ||
| getCurrentDirectory: () => "", | ||
| getDefaultLibFileName: () => "lib.2015.core.d.ts", |
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 default lib filename does not match the bundled lib files. Returns "lib.2015.core.d.ts" but the BUNDLED_LIBS map uses "lib.es2015.core.d.ts" as the key. This mismatch causes TypeScript to fail loading the default lib file, which means basic JavaScript types (Array, Object, String, etc.) won't be available during type checking.
Fix:
getDefaultLibFileName: () => "lib.es2015.core.d.ts",Or alternatively, update line 22 in COMPILER_OPTIONS to use "lib.2015.core.d.ts" and adjust the BUNDLED_LIBS map key accordingly.
| getDefaultLibFileName: () => "lib.2015.core.d.ts", | |
| getDefaultLibFileName: () => "lib.es2015.core.d.ts", |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
libs/@hashintel/petrinaut/src/core/checker/create-language-service-host.ts
Outdated
Show resolved
Hide resolved
| const tokenTuple = Array.from({ length: arc.weight }) | ||
| .fill(`Color_${sanitizedColorId}`) | ||
| .join(", "); | ||
| inputTypeProperties.push(` "${place.name}": [${tokenTuple}];`); |
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.
Bug: Duplicate place names generate invalid TypeScript type definitions
The generated TypeScript type definitions use place.name as property keys for the Input and Output types. Since place names are not guaranteed to be unique (the SDCPN type allows multiple places with the same name but different IDs), if a transition has arcs to two places that share the same name, the generated type literal will contain duplicate property names. This produces invalid TypeScript that causes the type-checker to fail with a syntax error when parsing the generated definitions.
Additional Locations (1)
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.
Is this fair? If so can we fix?
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.
It is not, or at least it should not.
We should never have multiple places with the same name, but at the moment we do not do this verification (that should be done in the part of the checker that does not do the type-checking).
In any case, this will not be the responsibility of the type-checker part.
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.
And if it results in a linting error, that's actually a good thing, even if it's not by-design.
libs/@hashintel/petrinaut/src/core/checker/create-sdcpn-language-service.ts
Outdated
Show resolved
Hide resolved
CiaranMn
left a comment
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.
Great addition! A few minor comments
|
|
||
| // Check all differential equations | ||
| for (const de of sdcpn.differentialEquations) { | ||
| const filePath = `differential_equations/${de.id}/code.ts`; |
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.
Can we have a generateCodeFilePath(id: string, type: ItemType): string function in create-sdcpn-language-service which is then used both here and when creating the files?
At the moment they are magic strings which need keeping in sync manually if they change.
Same applies to other file paths below.
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.
Yes much clearer indeed, I will add that.
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.
Done.
libs/@hashintel/petrinaut/src/core/checker/create-sdcpn-language-service.ts
Outdated
Show resolved
Hide resolved
| const tokenTuple = Array.from({ length: arc.weight }) | ||
| .fill(`Color_${sanitizedColorId}`) | ||
| .join(", "); | ||
| inputTypeProperties.push(` "${place.name}": [${tokenTuple}];`); |
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.
Is this fair? If so can we fix?
| }), | ||
|
|
||
| // Properties panel width | ||
| propertiesPanelWidth: 450, |
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.
Can we create a separate variable for this and diagnosticsPanelHeight to avoid repeating them and them potentially going out of sync?
i.e. somewhere up top in the file const propertiesPanelWidth = 450;
| // Position offsets (accounting for sidebar padding/margins) | ||
| const MIN_HEIGHT = 100; | ||
| const MAX_HEIGHT = 600; | ||
| const LEFT_SIDEBAR_WIDTH = 344; // 320px + 24px padding |
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.
Can we pull the sidebar width from where it's defined? And the padding from wherever that is?
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.
I tried to centralize all panel-related stuff together, including some styles, but ended up with bugs on styles.
If that is okay for you I'll add this on top of the PR fixing PandaCSS, or directly in the one that contains the BottomPanel (DiagnosticsPanel is already removed in my new PR)
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.
Okay sure – I don't see how just defining some numbers as variables rather than hard-coding them several times would cause style bugs, so hopefully easy enough to fix
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.
Not sure about that, but possibly one of the issues I had was:
- When the constant is in an external file, PandaCSS cannot statically analyze/create the style by just reading one file.
So this LEFT_SIDEBAR_WIDTH would either:
- Not be in the
css()anymore, but in an inline style - Replaced by a CSS variable.
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.
But given PandaCSS was not configured properly, I was not sure of what was causing the issue exactly.
| import { TransitionProperties } from "./transition-properties"; | ||
| import { TypeProperties } from "./type-properties"; | ||
|
|
||
| const startingWidth = 450; |
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.
Here we have 450 again, definitely this could go out of sync with the 450s in the editor store, should all draw from the same variable
libs/@hashintel/petrinaut/src/core/checker/create-sdcpn-language-service.ts
Show resolved
Hide resolved
libs/@hashintel/petrinaut/src/views/Editor/components/DiagnosticsPanel/diagnostics-panel.tsx
Outdated
Show resolved
Hide resolved
4188954 to
da1fbd9
Compare
| */ | ||
| function sanitizeColorId(colorId: string): string { | ||
| return colorId.replace(/[^a-zA-Z0-9_]/g, ""); | ||
| } |
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.
Bug: Color ID sanitization causes type name collisions
The sanitizeColorId function removes non-alphanumeric characters to create valid TypeScript identifiers, but this can cause type name collisions. If two colors have IDs that differ only in special characters (e.g., "color-1" and "color_1" both sanitize to "color1"), they'll generate types with identical names (Color_color1) in different files. When both types are imported in the same transition definition file, the second import shadows the first, causing incorrect type checking where places reference the wrong color's type structure.
Additional Locations (1)
| useEffect(() => { | ||
| const result = checkSDCPN(petriNetDefinition); | ||
| setCheckResult(result); | ||
| }, [petriNetDefinition]); |
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.
Bug: Redundant SDCPN check on initial component mount
The checkSDCPN function is called twice on initial mount. The useState initializer (line 22-24) calls it to set the initial state, and then the useEffect (lines 27-30) immediately runs after mount and calls it again because petriNetDefinition is in the dependency array. Since checkSDCPN creates a TypeScript language service internally via ts.createLanguageService, this double execution is expensive and causes unnecessary performance overhead on every component mount.
2fc0cf0 to
a5c3883
Compare
| useEffect(() => { | ||
| const result = checkSDCPN(petriNetDefinition); | ||
| setCheckResult(result); | ||
| }, [petriNetDefinition]); |
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.
Bug: Expensive type-checker runs twice on initial mount
The checkSDCPN function runs twice on initial mount — once in the useState initializer (line 22-24) and again immediately in the useEffect (line 27-30) since petriNetDefinition is in its dependency array. Since checkSDCPN creates a TypeScript language service and runs full diagnostic checks, this is an expensive operation to duplicate. Consider either using only the useState initializer with a ref to skip the first useEffect run, or initializing state as null and only using the effect.
| position: "fixed", | ||
| bottom: PANEL_MARGIN, | ||
| left: leftOffset, | ||
| right: PANEL_MARGIN, |
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.
Bug: DiagnosticsPanel overlaps PropertiesPanel due to unused width
The propertiesPanelWidth is stored in the editor state with a comment indicating it's "for DiagnosticsPanel positioning", and PropertiesPanel syncs its width to this value. However, DiagnosticsPanel never reads this value — it uses a fixed right: PANEL_MARGIN (12px) instead. When both panels are open, the DiagnosticsPanel extends underneath the PropertiesPanel (which has higher z-index), making a large portion of the diagnostics content visually obscured and potentially inaccessible.
Additional Locations (1)
| useEffect(() => { | ||
| const result = checkSDCPN(petriNetDefinition); | ||
| setCheckResult(result); | ||
| }, [petriNetDefinition]); |
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.
Bug: Type checker runs twice on component mount
The checkSDCPN function is called twice on initial mount. It's first invoked in the useState lazy initializer (line 22-24), then immediately again in the useEffect (line 27-30) which runs on mount because petriNetDefinition is in its dependency array. Since checkSDCPN creates a new TypeScript language service on each call (an expensive operation involving virtual file generation and ts.createLanguageService), this causes redundant computation and noticeable delay on initial load. The useEffect should skip its first run or the useState initializer should be removed.
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.
This will be fixed in H-5839

H-5840 (part of H-5684)
🌟 What is the purpose of this PR?
🔍 What does this change?
createSDCPNLanguageServicethat takes an SDCPN as input and lintscheckSDCPNfunction, that createsPre-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:
🐾 Next steps
🛡 What tests cover this?
createSDCPNfunction.❓ How to test this?
Use latest Petrinaut deployment (in thread)