Feature/variable browser rework#638
Open
Rdmkr wants to merge 5 commits intohighperformancecoder:masterfrom
Open
Feature/variable browser rework#638Rdmkr wants to merge 5 commits intohighperformancecoder:masterfrom
Rdmkr wants to merge 5 commits intohighperformancecoder:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new interactive SVG/D3 canvas and UI (components, directives, dialogs, data classes, styles), integrates it into the Variable Pane, updates Electron variable-popup sizing/route quoting, relaxes Angular template strictness, adds CSP meta tag and a few dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as SvgCanvasComponent
participant D3 as D3 Force Layout
participant Dialog as EditVariableDialog
participant Data as DataPoint
User->>UI: Load/provide unparkedData (DataPoint[])
UI->>D3: Initialize force simulation (nodes & links)
D3->>UI: Emit node positions
UI->>User: Render SVG (nodes, connectors, valuelines)
User->>UI: Drag node / interact
UI->>D3: Apply drag / update node
D3->>UI: Recompute forces
UI->>User: Update rendering
User->>UI: Right-click node
UI->>Dialog: Open EditVariableDialog with DataPoint
Dialog->>User: Collect edits
User->>Dialog: Submit
Dialog->>UI: Return updated DataPoint
UI->>User: Re-render with updated data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| dragged: () => { | ||
| if(this.draggable) { | ||
| const x = d3.event.x; | ||
| const y = d3.event.y; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
| @@ -0,0 +1,177 @@ | |||
| import { Dimensions } from '../classes/dimensions'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (16)
package.json (1)
12-12: Consider movingnxtodevDependencies.
nxis a build/monorepo orchestration tool that is not needed at runtime. Placing it independencieswill unnecessarily increase the production bundle size and installation time. This appears to be a duplicate of thenxalready present ingui-js/package.jsonunderdevDependencies.Proposed fix
"dependencies": { "bindings": "^1.5.0", - "nx": "^22.6.3", "typescript": "^5.8.3" }, "devDependencies": { "@types/node": "^24.0.10", + "nx": "^22.6.3", "node-addon-api": "^8.4.0", "ts-node": "^10.9.2" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 12, Move the "nx" entry out of the top-level "dependencies" and into "devDependencies" in package.json: remove the "nx": "^22.6.3" line from dependencies and add the same version under devDependencies (or skip adding if an equivalent "nx" already exists in gui-js/package.json devDependencies to avoid duplication), ensuring package.json no longer lists nx as a runtime dependency.gui-js/package.json (1)
195-198: Consider upgrading D3 to v7 and adding type definitions.A few observations on these dependencies:
D3 v5.16.0 is outdated (July 2020; current is v7.9.0). While the code in
dragdrop.directive.tsalready uses the modern D3 v6+ event callback pattern (.on('start', (d, e) => ...)), upgrading to v7 would bring bug fixes and improvements.Module version duplication:
@swimlane/ngx-graphv11 depends on modular D3 packages (d3-selection v3.x, d3-force v3.x, etc.), while the monolithicd3v5.16.0 bundle includes older versions of these same modules. This adds unnecessary bloat. Consider upgrading to D3 v7, which better aligns with modern D3 module versions.Missing TypeScript definitions: Add
@types/d3to thedevDependenciesfor improved type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/package.json` around lines 195 - 198, Update the d3 dependency in package.json from "d3": "^5.16.0" to a current v7 range (e.g., "^7.9.0") and add "@types/d3" to devDependencies; verify dragdrop.directive.ts event callbacks remain compatible with D3 v7 and, if you want to avoid module duplication with `@swimlane/ngx-graph`, consider replacing the monolithic "d3" with the specific modular d3-* packages used by the app so versions align with `@swimlane/ngx-graph` v11.gui-js/libs/ui-components/src/lib/svg-canvas/classes/connector.ts (1)
1-4: Consider using an interface instead of a class for this pure data structure.Since
Connectorhas no methods or constructor logic, an interface would be more idiomatic and avoids unnecessary runtime overhead.♻️ Proposed refactor
-export class Connector { - type: string; - position: (x: number, y: number, width: number, height: number) => [number,number]; -} +export interface Connector { + type: string; + position: (x: number, y: number, width: number, height: number) => [number, number]; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/connector.ts` around lines 1 - 4, The Connector is a pure data shape with no methods or constructor, so replace the exported class Connector with an exported interface Connector that declares the same properties (type: string and position: (x: number, y: number, width: number, height: number) => [number, number]); update any code that instantiates Connector with new Connector(...) to instead use object literals or factories returning Connector and ensure any type annotations/imports still reference the Connector interface; remove the class declaration to avoid runtime overhead.gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.html (1)
25-25: Inconsistent indentation on closing tags.The closing
</ng-container>on line 25 and</div>on line 34 are not aligned with their opening tags, reducing readability.✨ Proposed fix
<mat-form-field class="maximuminput"><input type="number" matInput [(ngModel)]="result.maximum"></mat-form-field> -</ng-container> + </ng-container><mat-form-field class="descriptioninput"><textarea matInput [(ngModel)]="result.description"></textarea></mat-form-field> -</div> + </div>Also applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.html` at line 25, Fix the inconsistent indentation of the closing tags in the template: align the closing </ng-container> and the closing </div> with their respective opening tags so indentation level matches the block they close (adjust whitespace before </ng-container> and </div> to the same indentation as the matching opening <ng-container ...> and <div ...> lines), ensuring consistent nesting and readability in editvariabledialog.component.html.gui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.html (1)
6-7: Callingdragfunctions(p, i)in the template creates new function instances on each change detection cycle.This may cause unnecessary re-renders since Angular compares by reference. Consider caching the drag functions or using
trackByto mitigate performance impact if the midpoint count is large.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.html` around lines 6 - 7, The template calls dragfunctions(p, i) inside the *ngFor which creates new function instances every change detection cycle; to fix, move function creation out of the template by precomputing and caching per-point handlers in the component (e.g., create an array like dragHandlers[] populated/updated whenever dotPositions change and expose a getter or property to bind [dragfunctions]="dragHandlers[i]"), and/or add a trackBy function for the *ngFor (referencing dotPositions, the midpoint element and the dragfunctions method) so Angular can reuse nodes instead of recreating them on each CD cycle.gui-js/libs/ui-components/src/lib/svg-canvas/classes/dimensions.ts (1)
1-8: Consider using an interface and more specific tuple types for fixed-length arrays.If
boundingbox,labelbox, etc. have fixed lengths (e.g.,[x, y, width, height]), tuple types would provide better type safety. Also, an interface would be more idiomatic for a pure data structure.♻️ Proposed refactor
-export class Dimensions { - length: number; - boundingbox: number[]; - labelbox: number[]; - valuebox: number[]; - exponentbox: number[]; - valueline?: number[][]; -} +export interface Dimensions { + length: number; + boundingbox: [number, number, number, number]; // [x, y, width, height] + labelbox: [number, number, number, number]; + valuebox: [number, number, number, number]; + exponentbox: [number, number, number, number]; + valueline?: number[][]; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/dimensions.ts` around lines 1 - 8, Replace the Dimensions class with a plain interface named Dimensions and tighten the array types to fixed-length tuples where appropriate: change boundingbox, labelbox, valuebox, and exponentbox from number[] to [number, number, number, number] (or the exact 4-tuple representing x,y,width,height), keep length: number, and change valueline?: number[][] to valueline?: [number, number][] to express pairs; update any usages that construct or type-check Dimensions to use the interface instead of the class name.gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.ts (1)
32-36: UnusedMatDialoginjection.The
MatDialogservice is injected but never used. Remove it to clean up the constructor.🔧 Proposed fix
- constructor(public dialog: MatDialog, - public dialogRef: MatDialogRef<EditVariableDialog>, + constructor(public dialogRef: MatDialogRef<EditVariableDialog>, `@Inject`(MAT_DIALOG_DATA) public data: EditVariableDialogData) {Also update the import:
-import { MatDialogRef, MAT_DIALOG_DATA, MatDialog } from '@angular/material/dialog'; +import { MatDialogRef, MAT_DIALOG_DATA } from '@angular/material/dialog';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.ts` around lines 32 - 36, The constructor for EditVariableDialog injects an unused MatDialog; remove the MatDialog parameter from the constructor signature and the corresponding import to clean up the code (leave MatDialogRef, MAT_DIALOG_DATA and EditVariableDialogData injections intact), and ensure the class member this.result = data remains unchanged so EditVariableDialog and its constructor still set result from the injected data.gui-js/libs/ui-components/src/lib/svg-canvas/directives/connector.directive.ts (2)
17-19: Consider usingSVGElementtype for SVG element references.This directive is applied to SVG
<circle>elements (per the template), butElementRef<HTMLElement>is specified. For better type accuracy, useSVGElement.🔧 Proposed fix
- constructor(private el: ElementRef<HTMLElement>) { + constructor(private el: ElementRef<SVGElement>) { el.nativeElement.style.fillOpacity = '0'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/directives/connector.directive.ts` around lines 17 - 19, The constructor of the ConnectorDirective currently types the injected element as ElementRef<HTMLElement>, but the directive is applied to SVG <circle> elements; update the constructor signature to use ElementRef<SVGElement> (or ElementRef<SVGCircleElement> for even stricter typing) so DOM APIs like style and SVG-specific properties are correctly typed; locate the constructor in connector.directive.ts and replace ElementRef<HTMLElement> with ElementRef<SVGElement> (adjust any downstream usages if necessary).
7-15: Unused event parameters can be removed.The
eventparameter is declared but never used in bothmouseenterandmouseouthandlers. Either remove it from the method signature or prefix with underscore if you need to keep the type annotation.🔧 Proposed fix
- `@HostListener`('mouseenter', ['$event']) - mouseenter(event: any) { + `@HostListener`('mouseenter') + mouseenter() { this.el.nativeElement.style.fillOpacity = '0.5'; } - `@HostListener`('mouseout', ['$event']) - mouseout(event: any) { + `@HostListener`('mouseout') + mouseout() { this.el.nativeElement.style.fillOpacity = '0'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/directives/connector.directive.ts` around lines 7 - 15, The HostListener handlers mouseenter and mouseout in connector.directive.ts declare an unused parameter (event); remove the unused parameter from both method signatures (or rename to _event if you must keep it) so the methods become mouseenter() and mouseout() to satisfy linting and avoid unused-parameter warnings while keeping the existing HostListener decorators intact.gui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.ts (1)
13-14: Add type annotation for thedatainput.The
datainput lacks a type annotation. Based on usage context, it should be typed asDataPointfor better type safety and IDE support.🔧 Proposed fix
+import { DataPoint } from '../classes/datapoint'; + // ... `@Input`() - data; + data: DataPoint;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.ts` around lines 13 - 14, The `@Input` property data is missing a type annotation; update the ElementLabelComponent's `@Input`() data property to use the DataPoint type (i.e., change the declaration of the data input to be typed as DataPoint) so the component gains compile-time type safety and IDE support; ensure you import DataPoint if needed and update any usages within methods like render/formatLabel that assume properties on DataPoint.gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.html (2)
32-34: Connector position is computed twice per circle.The expression
c.position(0, 0, d.dimensions.boundingbox[2] / 2, d.dimensions.boundingbox[3] / 2)is called twice to extract[0]and[1]. Consider caching this in the component or using a computed property to avoid redundant calculations during change detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.html` around lines 32 - 34, The template calls c.position(...) twice for each circle which duplicates work; fix by computing the position once and reusing it — either add a helper on the component (e.g., getConnectorPos(connector, d) or computeConnectorPosition(connector, d) that returns [x,y]) and bind [attr.cx] and [attr.cy] to its result, or introduce a template variable (compute once with an ng-container or let-pos) and use pos[0] and pos[1]; keep connectorClicked(c, $event) and the connector r/style bindings unchanged.
26-29: Use strict equality for exponent comparison.Line 26 uses
d.exponent != 0(loose equality). Prefer!== 0for strict comparison to avoid unexpected type coercion.🔧 Proposed fix
-<foreignObject *ngIf="d.dimensions.exponentbox && d.exponent != 0" [attr.x]="d.dimensions.exponentbox[0]" [attr.y]="d.dimensions.exponentbox[1]" [attr.width]="d.dimensions.exponentbox[2]" [attr.height]="d.dimensions.exponentbox[3]" +<foreignObject *ngIf="d.dimensions.exponentbox && d.exponent !== 0" [attr.x]="d.dimensions.exponentbox[0]" [attr.y]="d.dimensions.exponentbox[1]" [attr.width]="d.dimensions.exponentbox[2]" [attr.height]="d.dimensions.exponentbox[3]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.html` around lines 26 - 29, The template uses a loose equality check `d.exponent != 0` inside the foreignObject *ngIf; change this to a strict comparison `d.exponent !== 0` to avoid type-coercion edge cases. Update the *ngIf expression that references `d.dimensions.exponentbox` and `d.exponent` (the same expression that drives rendering of the exponent via `exponentEquation(d.exponent)` and the transform using `isUpsideDown(d)`) to use `!==` for the exponent check.gui-js/libs/ui-components/src/lib/svg-canvas/classes/elementtype.ts (1)
4-10: Consider using an interface instead of a class for type definition.
ElementTypedefines a shape without implementation logic or initialization. An interface would be more idiomatic for TypeScript and avoids potential strict-mode issues with uninitialized properties.🔧 Proposed fix
import { Connector } from './connector'; import { Dimensions } from './dimensions'; -export class ElementType { - name: string; - points: (x: number, y: number, width: number, height: number) => string; - size: number[]; - connectors: Connector[]; - getDimensions: (length: number) => Dimensions; -} +export interface ElementType { + name: string; + points: (x: number, y: number, width: number, height: number) => string; + size: number[]; + connectors: Connector[]; + getDimensions: (length: number) => Dimensions; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/elementtype.ts` around lines 4 - 10, Replace the class ElementType with a TypeScript interface because it only describes a shape and has no implementation or initialization; redefine ElementType as an interface containing the same members (name: string, points: (x: number, y: number, width: number, height: number) => string, size: number[], connectors: Connector[], getDimensions: (length: number) => Dimensions) and update any usages that rely on it being a value to use the type-only interface instead; ensure imports/exports remain consistent and that Connector and Dimensions types are referenced unchanged.gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas-demo.component.ts (1)
21-30: Consider adding type annotation for the template parameter.Using
anyfor the template parameter loses type safety. Consider defining an interface for the template shape.🔧 Proposed fix
+interface VariableTemplate { + symbol: string; + name: string; + description?: string; + type: any; // Or the specific ElementType if available +} + // In the map call: - ].map((template: any, i: number) => { + ].map((template: VariableTemplate, i: number) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas-demo.component.ts` around lines 21 - 30, The map callback currently types the parameter as any which loses type safety; define a Template interface (e.g., SvgTemplate with the expected fields used by SvgCanvasHelper.createData) and replace the any in the array.map callback signature with that interface (e.g., .map((template: SvgTemplate, i: number) => { ... }); update/create the interface near this component and import or export it if needed so SvgCanvasHelper.createData and svg-canvas-demo.component.ts both use the same strong type.gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.scss (1)
7-11: Remove unexpected empty lines before declarations to satisfy Stylelint.The static analysis tool flags empty lines before declarations at lines 8, 18, and 20. If the project enforces this rule, consider removing these empty lines.
🔧 Proposed fix
`#variable-pane-wrapper` { height: 100vh; width: 100vw; border: 1px solid $toolbar-box; - display: flex; flex-direction: column; justify-content: space-between; align-items: stretch; }Similarly for
.variable-pane:.variable-pane { height: 400vh; width: 100%; - flex: 1; - overflow-x: auto; overflow-y: hidden;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.scss` around lines 7 - 11, The Stylelint complaint is caused by unexpected blank lines before property declarations in the .variable-pane stylesheet; open variable-pane.component.scss and remove the empty lines immediately preceding the property declarations inside the .variable-pane (and any nested selectors) so rules like display, flex-direction, justify-content and align-items have no blank line before them; save and re-run lint to confirm the spacing issue is resolved.gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.css (1)
22-24: Hard-coded background color may not adapt to themes.Consider using a CSS variable or Angular Material theme color instead of the hard-coded
#eaf2ffto ensure the dialog adapts to different themes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.css` around lines 22 - 24, The rule hard-codes mat-select { background-color: `#eaf2ff`; } which won't adapt to themes; change it to use a theme-aware value such as a CSS variable or Angular Material theme token instead: replace the literal with something like background-color: var(--app-select-bg, `#eaf2ff`); (or, if you can switch to SCSS theming, use the Angular Material palette/mixins to apply mat-color for the desired palette) and ensure the theme stylesheet sets --app-select-bg per light/dark theme so the mat-select selector picks up the correct color.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/datapoint.ts`:
- Around line 5-22: The DataPoint model currently declares "label" but the rest
of the codebase constructs/uses a "name" property (see VariablePaneComponent and
SvgCanvasComponent referencing d.name), causing a type/runtime mismatch; update
DataPoint (the class exported as DataPoint in datapoint.ts) to include a "name:
string" property (and optionally remove or alias "label" if you want a single
canonical field) so the declared type matches objects created/edited elsewhere
(ensure usages that expect d.name keep working).
In `@gui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.ts`:
- Around line 86-96: The drag handler is ignoring vertical movement by using a
hardcoded point [x, 1000]; update the probe point to use the live drag Y from
d3.event (replace [x, 1000] with [x, y]) so getAngle(...) computes using the
actual drag position; change the point construction in the dragged lambda (where
draggable, x, y, point, getAngle, dotsAt[i], and pointMoved are referenced) to
use the captured y variable.
- Around line 5-10: D3LineComponent is missing the standalone flag but already
uses the imports metadata, so add standalone: true to the `@Component` metadata
for D3LineComponent to make it a standalone component that can be referenced
from SvgCanvasComponent's imports array; update the `@Component` decorator for
D3LineComponent (the class named D3LineComponent) to include standalone: true
while keeping its existing imports, selector, templateUrl, and styleUrls intact.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.html`:
- Around line 4-6: The template calls data.value.toFixed(4) without guarding for
null/undefined; update the elementlabel.component.html expression (the line
rendering {{data.value.toFixed(4)}}) to guard against missing values—either use
Angular safe navigation/optional chaining and a default (e.g.,
{{data?.value?.toFixed(4) ?? ''}} or {{data.value?.toFixed(4) ?? ''}} if
allowed) or wrap the display in an *ngIf that checks data && data.value !==
undefined; also ensure the component's `@Input`() data is appropriately typed
(e.g., DataPoint | undefined) in the ElementLabelComponent so the template and
type system reflect the possible absence of value.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.ts`:
- Around line 19-21: The fontSize getter can divide by zero when this.lines ===
0; modify the fontSize logic in the elementlabel.component (the fontSize getter)
to guard against zero/negative values (e.g. use Math.max(this.lines, 1) or
return a sensible default like '20px' when this.lines is falsy) so it never
returns "Infinitypx" and always produces valid CSS.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.css`:
- Around line 9-20: The CSS in .select-container defines three grid columns via
grid-template-columns: 20% 70% 10% but grid-template-areas rows each only
provide two areas, causing the third column to never be used; fix by either
removing the extra column spec (change grid-template-columns to two columns) or
update grid-template-areas to include a third placeholder (e.g., add a '.' or a
third area name at the end of each row) so the columns and areas match for
.select-container.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.ts`:
- Around line 34-36: The constructor currently assigns the injected
EditVariableDialogData by reference (this.result = data) which causes edits to
mutate the original; change it to clone the incoming data into this.result so
cancel can discard edits (e.g., use a deep clone such as structuredClone(data)
or JSON.parse(JSON.stringify(data)) to copy nested properties) and preserve
types expected by EditVariableDialogData; update the constructor that injects
MAT_DIALOG_DATA accordingly so two-way binding operates on the cloned object
rather than the original.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/directives/dragdrop.directive.ts`:
- Around line 8-16: The setter dragfunctions(df) is re-attaching D3 drag
handlers on every change; move the one-time initialization into ngAfterViewInit
(create the d3.drag() and call selection.call(...) there) and keep a reference
to the drag behavior on the directive (e.g., this._dragBehavior) so you do not
reattach on every setter call; if you must support runtime updates to handlers,
implement OnChanges to detect actual changes, remove previous handlers before
reattaching (use d3 selection.on('.drag', null) or call selection.on with the
appropriate namespace) and implement ngOnDestroy to clean up the drag behavior
and listeners.
- Around line 12-15: The drag event handlers pass an unused parameter `d` to
functions that are defined without parameters (see obj.call(...
d3.drag().on('start', (d, e) => df.dragstarted(d)) ...)) — remove the unused `d`
argument so the callbacks match the signatures of df.dragstarted, df.dragged,
and df.dragended and rely on the closure/ d3.event as intended; update the three
.on(...) calls to call df.dragstarted, df.dragged, and df.dragended directly (or
use () => df.dragstarted()) so there are no unused parameters in the
dragdrop.directive.ts binding.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.ts`:
- Around line 479-489: The dialog close handler currently updates only raw
fields on the node variable d (symbol, name, description, value/minimum/maximum)
but does not recompute derived geometry, so connector geometry, actualLength and
dimensions remain stale; after mutating d inside the
dialogRef.afterClosed().subscribe callback, call the geometry recomputation used
elsewhere (e.g., SvgCanvasHelper.createData(d) or the component method that
wraps it) and merge/assign its returned connectors, actualLength and dimensions
back onto d (or replace d with the returned data) so the node width and
connector geometry update immediately after edits.
- Around line 149-158: The commented-out initialization causes rectCollideForce
to be undefined and later calls to this.rectCollideForce.sizeModifier(...) from
the drag handlers to throw; restore or safely initialize it in ngAfterViewInit
by creating the rectCollide force (call this.rectCollide() to assign
this.rectCollideForce), configure its strength (e.g., strength(3)) and, if
required, reattach it to the simulation (this.simulation =
d3.forceSimulation().force('rectcollide',
this.rectCollideForce).nodes(this.unparkedData)); alternatively, add a defensive
guard in the drag handlers (check if this.rectCollideForce exists before calling
sizeModifier) so the first drag won't throw.
- Around line 494-498: getConnectorPos is using c.data.type.boundingbox which is
undefined because createData stores bounding box under c.data.dimensions; update
getConnectorPos to read the bounding box from c.data.dimensions (e.g. use
c.data.dimensions.boundingbox[2] and [3] when computing points), keep the
subsequent rotatePoint(...) and return shape the same, and ensure callers of
getConnectorPos still receive the same [x,y,rotation] tuple.
- Line 8: The import for MatFormField is using the wrong package; change any
import of MatFormField from '@angular/material/input' to
'@angular/material/form-field' (and update any related component/module imports
in the same file that reference MatFormField) so the symbol resolves correctly
in SvgCanvasComponent (and any other components in this file that import
MatFormField).
In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts`:
- Around line 142-145: Clamp the computed columnLength to a minimum of 1 before
using it to compute numberOfColumns to avoid division-by-zero/infinite loops:
replace the raw Math.round(this.variablePaneContainer.clientHeight / 50) with
something like const columnLength = Math.max(1,
Math.round(this.variablePaneContainer.clientHeight / 50)); then compute
numberOfColumns using svgData.length and columnLength (and consider using
Math.ceil or Math.floor as appropriate) so the for loop over numberOfColumns
(referencing variablePaneContainer, columnLength, numberOfColumns, svgData) is
always bounded.
- Around line 130-163: Guard the setPositions() method against running before
initialization by returning early when required fields are not ready: check that
this.allSvgData is defined and is an array, this.svgCanvas is initialized, and
this.variablePaneContainer (and the form elements used) exist before proceeding;
if any are absent, simply return. Update setPositions() (referencing
setPositions, allSvgData, svgCanvas, variablePaneContainer, containerWidth$,
svgData$, svgCanvas.setDimensions) to perform these null/undefined checks at the
top so the resize listener can safely call it before initEvents() completes.
---
Nitpick comments:
In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/connector.ts`:
- Around line 1-4: The Connector is a pure data shape with no methods or
constructor, so replace the exported class Connector with an exported interface
Connector that declares the same properties (type: string and position: (x:
number, y: number, width: number, height: number) => [number, number]); update
any code that instantiates Connector with new Connector(...) to instead use
object literals or factories returning Connector and ensure any type
annotations/imports still reference the Connector interface; remove the class
declaration to avoid runtime overhead.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/dimensions.ts`:
- Around line 1-8: Replace the Dimensions class with a plain interface named
Dimensions and tighten the array types to fixed-length tuples where appropriate:
change boundingbox, labelbox, valuebox, and exponentbox from number[] to
[number, number, number, number] (or the exact 4-tuple representing
x,y,width,height), keep length: number, and change valueline?: number[][] to
valueline?: [number, number][] to express pairs; update any usages that
construct or type-check Dimensions to use the interface instead of the class
name.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/elementtype.ts`:
- Around line 4-10: Replace the class ElementType with a TypeScript interface
because it only describes a shape and has no implementation or initialization;
redefine ElementType as an interface containing the same members (name: string,
points: (x: number, y: number, width: number, height: number) => string, size:
number[], connectors: Connector[], getDimensions: (length: number) =>
Dimensions) and update any usages that rely on it being a value to use the
type-only interface instead; ensure imports/exports remain consistent and that
Connector and Dimensions types are referenced unchanged.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.html`:
- Around line 6-7: The template calls dragfunctions(p, i) inside the *ngFor
which creates new function instances every change detection cycle; to fix, move
function creation out of the template by precomputing and caching per-point
handlers in the component (e.g., create an array like dragHandlers[]
populated/updated whenever dotPositions change and expose a getter or property
to bind [dragfunctions]="dragHandlers[i]"), and/or add a trackBy function for
the *ngFor (referencing dotPositions, the midpoint element and the dragfunctions
method) so Angular can reuse nodes instead of recreating them on each CD cycle.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.ts`:
- Around line 13-14: The `@Input` property data is missing a type annotation;
update the ElementLabelComponent's `@Input`() data property to use the DataPoint
type (i.e., change the declaration of the data input to be typed as DataPoint)
so the component gains compile-time type safety and IDE support; ensure you
import DataPoint if needed and update any usages within methods like
render/formatLabel that assume properties on DataPoint.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.css`:
- Around line 22-24: The rule hard-codes mat-select { background-color: `#eaf2ff`;
} which won't adapt to themes; change it to use a theme-aware value such as a
CSS variable or Angular Material theme token instead: replace the literal with
something like background-color: var(--app-select-bg, `#eaf2ff`); (or, if you can
switch to SCSS theming, use the Angular Material palette/mixins to apply
mat-color for the desired palette) and ensure the theme stylesheet sets
--app-select-bg per light/dark theme so the mat-select selector picks up the
correct color.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.html`:
- Line 25: Fix the inconsistent indentation of the closing tags in the template:
align the closing </ng-container> and the closing </div> with their respective
opening tags so indentation level matches the block they close (adjust
whitespace before </ng-container> and </div> to the same indentation as the
matching opening <ng-container ...> and <div ...> lines), ensuring consistent
nesting and readability in editvariabledialog.component.html.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.ts`:
- Around line 32-36: The constructor for EditVariableDialog injects an unused
MatDialog; remove the MatDialog parameter from the constructor signature and the
corresponding import to clean up the code (leave MatDialogRef, MAT_DIALOG_DATA
and EditVariableDialogData injections intact), and ensure the class member
this.result = data remains unchanged so EditVariableDialog and its constructor
still set result from the injected data.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/directives/connector.directive.ts`:
- Around line 17-19: The constructor of the ConnectorDirective currently types
the injected element as ElementRef<HTMLElement>, but the directive is applied to
SVG <circle> elements; update the constructor signature to use
ElementRef<SVGElement> (or ElementRef<SVGCircleElement> for even stricter
typing) so DOM APIs like style and SVG-specific properties are correctly typed;
locate the constructor in connector.directive.ts and replace
ElementRef<HTMLElement> with ElementRef<SVGElement> (adjust any downstream
usages if necessary).
- Around line 7-15: The HostListener handlers mouseenter and mouseout in
connector.directive.ts declare an unused parameter (event); remove the unused
parameter from both method signatures (or rename to _event if you must keep it)
so the methods become mouseenter() and mouseout() to satisfy linting and avoid
unused-parameter warnings while keeping the existing HostListener decorators
intact.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas-demo.component.ts`:
- Around line 21-30: The map callback currently types the parameter as any which
loses type safety; define a Template interface (e.g., SvgTemplate with the
expected fields used by SvgCanvasHelper.createData) and replace the any in the
array.map callback signature with that interface (e.g., .map((template:
SvgTemplate, i: number) => { ... }); update/create the interface near this
component and import or export it if needed so SvgCanvasHelper.createData and
svg-canvas-demo.component.ts both use the same strong type.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.html`:
- Around line 32-34: The template calls c.position(...) twice for each circle
which duplicates work; fix by computing the position once and reusing it —
either add a helper on the component (e.g., getConnectorPos(connector, d) or
computeConnectorPosition(connector, d) that returns [x,y]) and bind [attr.cx]
and [attr.cy] to its result, or introduce a template variable (compute once with
an ng-container or let-pos) and use pos[0] and pos[1]; keep connectorClicked(c,
$event) and the connector r/style bindings unchanged.
- Around line 26-29: The template uses a loose equality check `d.exponent != 0`
inside the foreignObject *ngIf; change this to a strict comparison `d.exponent
!== 0` to avoid type-coercion edge cases. Update the *ngIf expression that
references `d.dimensions.exponentbox` and `d.exponent` (the same expression that
drives rendering of the exponent via `exponentEquation(d.exponent)` and the
transform using `isUpsideDown(d)`) to use `!==` for the exponent check.
In
`@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.scss`:
- Around line 7-11: The Stylelint complaint is caused by unexpected blank lines
before property declarations in the .variable-pane stylesheet; open
variable-pane.component.scss and remove the empty lines immediately preceding
the property declarations inside the .variable-pane (and any nested selectors)
so rules like display, flex-direction, justify-content and align-items have no
blank line before them; save and re-run lint to confirm the spacing issue is
resolved.
In `@gui-js/package.json`:
- Around line 195-198: Update the d3 dependency in package.json from "d3":
"^5.16.0" to a current v7 range (e.g., "^7.9.0") and add "@types/d3" to
devDependencies; verify dragdrop.directive.ts event callbacks remain compatible
with D3 v7 and, if you want to avoid module duplication with
`@swimlane/ngx-graph`, consider replacing the monolithic "d3" with the specific
modular d3-* packages used by the app so versions align with `@swimlane/ngx-graph`
v11.
In `@package.json`:
- Line 12: Move the "nx" entry out of the top-level "dependencies" and into
"devDependencies" in package.json: remove the "nx": "^22.6.3" line from
dependencies and add the same version under devDependencies (or skip adding if
an equivalent "nx" already exists in gui-js/package.json devDependencies to
avoid duplication), ensuring package.json no longer lists nx as a runtime
dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 356480d3-72d5-424f-a55d-3b1e8aaafce0
⛔ Files ignored due to path filters (2)
gui-js/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.tsgui-js/apps/minsky-web/src/index.htmlgui-js/libs/ui-components/src/lib/svg-canvas/classes/connector.tsgui-js/libs/ui-components/src/lib/svg-canvas/classes/datapoint.tsgui-js/libs/ui-components/src/lib/svg-canvas/classes/dimensions.tsgui-js/libs/ui-components/src/lib/svg-canvas/classes/elementtype.tsgui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.cssgui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.htmlgui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.tsgui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.cssgui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.htmlgui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.tsgui-js/libs/ui-components/src/lib/svg-canvas/constants/svg-constants.tsgui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.cssgui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.htmlgui-js/libs/ui-components/src/lib/svg-canvas/dialogs/editvariabledialog.component.tsgui-js/libs/ui-components/src/lib/svg-canvas/directives/connector.directive.tsgui-js/libs/ui-components/src/lib/svg-canvas/directives/dragdrop.directive.tsgui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas-demo.component.cssgui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas-demo.component.htmlgui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas-demo.component.tsgui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.cssgui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.htmlgui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.tsgui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.htmlgui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.scssgui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.tsgui-js/libs/ui-components/src/lib/wiring/variable/variable.component.tsgui-js/package.jsongui-js/tsconfig.base.jsonpackage.json
Comment on lines
+5
to
+22
| export class DataPoint { | ||
| label: string; | ||
| x?: number; | ||
| y?: number; | ||
| fx?: number; | ||
| fy?: number; | ||
| value: number; | ||
| exponent: number; | ||
| rotation: number; | ||
| anchorx?: number; | ||
| anchory?: number; | ||
| type: ElementType; | ||
| connectors: Connector[]; | ||
| canvasPosition; | ||
| symbol; | ||
| dimensions: Dimensions; | ||
| hidden = false; | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align this model with the field name the rest of the PR uses.
VariablePaneComponent creates these objects with name (gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts Line 118), and SvgCanvasComponent later edits d.name (gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.ts Lines 469-482). Keeping only label here means the declared type no longer matches the runtime shape, which is an easy way to end up with stale/broken name bindings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/classes/datapoint.ts` around
lines 5 - 22, The DataPoint model currently declares "label" but the rest of the
codebase constructs/uses a "name" property (see VariablePaneComponent and
SvgCanvasComponent referencing d.name), causing a type/runtime mismatch; update
DataPoint (the class exported as DataPoint in datapoint.ts) to include a "name:
string" property (and optionally remove or alias "label" if you want a single
canonical field) so the declared type matches objects created/edited elsewhere
(ensure usages that expect d.name keep working).
Comment on lines
+5
to
+10
| @Component({ | ||
| selector: '[d3line]', | ||
| templateUrl: './d3line.component.html', | ||
| styleUrls: ['./d3line.component.css'], | ||
| imports: [CommonModule] | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Angular, is the importsmetadata field valid on a component withoutstandalone: true, and can such a component be imported directly into another standalone component's imports array?
💡 Result:
No, the imports metadata field is not valid on a component without standalone: true. It is only available for standalone components. Additionally, a non-standalone component cannot be imported directly into another standalone component's imports array. Non-standalone components must be declared and exported from an NgModule, and that NgModule must be imported into the standalone component's imports array instead.
Citations:
- 1: https://stackoverflow.com/questions/79204594/imports-is-only-valid-on-a-component-that-is-standalone
- 2: https://next.angular.io/api/core/Component
- 3: https://angular.dev/api/core/Component
- 4: https://stackoverflow.com/questions/79251768/angular-19-imports-is-only-valid-on-a-component-that-is-standalone
- 5: https://stackoverflow.com/questions/75032062/how-to-import-not-standalone-components-to-a-standalone-component-in-angular-14
🏁 Script executed:
# Find the SvgCanvasComponent file
fd -e ts SvgCanvasRepository: highperformancecoder/minsky
Length of output: 53
🏁 Script executed:
# Search for D3LineComponent imports in the codebase
rg "D3LineComponent" --type ts -B 2 -A 2Repository: highperformancecoder/minsky
Length of output: 1957
🏁 Script executed:
# Check the SvgCanvasComponent definition to see its imports
rg "selector.*svg-canvas" --type ts -B 5 -A 15Repository: highperformancecoder/minsky
Length of output: 5169
Mark this component as standalone before importing it directly.
SvgCanvasComponent imports D3LineComponent in its standalone imports array, but D3LineComponent omits standalone: true. Angular only allows this wiring when the child component is declared as standalone. Currently, D3LineComponent has the imports metadata field (used only for standalone components) without the standalone: true flag, making it a non-standalone component that cannot be directly imported into another component's imports array.
Suggested fix
`@Component`({
selector: '[d3line]',
+ standalone: true,
templateUrl: './d3line.component.html',
styleUrls: ['./d3line.component.css'],
imports: [CommonModule]
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Component({ | |
| selector: '[d3line]', | |
| templateUrl: './d3line.component.html', | |
| styleUrls: ['./d3line.component.css'], | |
| imports: [CommonModule] | |
| }) | |
| `@Component`({ | |
| selector: '[d3line]', | |
| standalone: true, | |
| templateUrl: './d3line.component.html', | |
| styleUrls: ['./d3line.component.css'], | |
| imports: [CommonModule] | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.ts`
around lines 5 - 10, D3LineComponent is missing the standalone flag but already
uses the imports metadata, so add standalone: true to the `@Component` metadata
for D3LineComponent to make it a standalone component that can be referenced
from SvgCanvasComponent's imports array; update the `@Component` decorator for
D3LineComponent (the class named D3LineComponent) to include standalone: true
while keeping its existing imports, selector, templateUrl, and styleUrls intact.
Comment on lines
+86
to
+96
| dragged: () => { | ||
| if(this.draggable) { | ||
| const x = d3.event.x; | ||
| const y = d3.event.y; | ||
| const point = [x,1000]; | ||
|
|
||
| const a1 = Math.max(0, Math.PI / 2 - this.getAngle(end, start, point)); | ||
| const a2 = Math.max(0, Math.PI / 2 - this.getAngle(start, end, point)); | ||
|
|
||
| this.dotsAt[i] = a1 / (a1 + a2); | ||
| this.pointMoved.emit(this.dotsAt[i]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the repo's D3 version and locate legacy global-event usage.
fd -i 'package.json' . -x rg -n '"d3"\s*:' {}
fd -i 'package-lock.json' . -x rg -n '"d3"\s*:' {}
rg -n -C2 "d3\.event|dragstarted:|dragged:|dragended:" gui-js/libs/ui-components/src/lib/svg-canvasRepository: highperformancecoder/minsky
Length of output: 6980
Replace hardcoded Y coordinate with live drag position.
Line 89 captures y from the drag event, but line 90 hardcodes the probe point to [x, 1000], discarding the captured Y coordinate. This causes dotsAt[i] to only respond to horizontal drag movements instead of using the full 2D drag position.
The codebase uses D3 v5.16.0, where d3.event is correctly available during drag callbacks, so the current implementation works as-is. However, the hardcoded Y value appears unintentional given the pattern elsewhere in the component.
Fix
- const point = [x,1000];
+ const point = [x, y];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dragged: () => { | |
| if(this.draggable) { | |
| const x = d3.event.x; | |
| const y = d3.event.y; | |
| const point = [x,1000]; | |
| const a1 = Math.max(0, Math.PI / 2 - this.getAngle(end, start, point)); | |
| const a2 = Math.max(0, Math.PI / 2 - this.getAngle(start, end, point)); | |
| this.dotsAt[i] = a1 / (a1 + a2); | |
| this.pointMoved.emit(this.dotsAt[i]); | |
| dragged: () => { | |
| if(this.draggable) { | |
| const x = d3.event.x; | |
| const y = d3.event.y; | |
| const point = [x, y]; | |
| const a1 = Math.max(0, Math.PI / 2 - this.getAngle(end, start, point)); | |
| const a2 = Math.max(0, Math.PI / 2 - this.getAngle(start, end, point)); | |
| this.dotsAt[i] = a1 / (a1 + a2); | |
| this.pointMoved.emit(this.dotsAt[i]); |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 89-89: Unused variable, import, function or class
Unused variable y.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/components/d3line.component.ts`
around lines 86 - 96, The drag handler is ignoring vertical movement by using a
hardcoded point [x, 1000]; update the probe point to use the live drag Y from
d3.event (replace [x, 1000] with [x, y]) so getAngle(...) computes using the
actual drag position; change the point construction in the dragged lambda (where
draggable, x, y, point, getAngle, dotsAt[i], and pointMoved are referenced) to
use the captured y variable.
Comment on lines
+4
to
+6
| <div *ngIf="lines > 1" class="valuelabel" [style.fontSize]="fontSize"> | ||
| {{data.value.toFixed(4)}} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Guard against undefined data.value before calling .toFixed(4).
The data input is untyped (@Input() data;), and DataPoint.value may not always be initialized. Calling .toFixed(4) on undefined or null will throw a TypeError at runtime.
🛡️ Proposed fix: add null guard
<div *ngIf="lines > 1" class="valuelabel" [style.fontSize]="fontSize">
- {{data.value.toFixed(4)}}
+ {{data.value != null ? data.value.toFixed(4) : ''}}
</div>Alternatively, consider using the optional chaining operator if the Angular compiler version supports it:
{{data.value?.toFixed(4) ?? ''}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div *ngIf="lines > 1" class="valuelabel" [style.fontSize]="fontSize"> | |
| {{data.value.toFixed(4)}} | |
| </div> | |
| <div *ngIf="lines > 1" class="valuelabel" [style.fontSize]="fontSize"> | |
| {{data.value != null ? data.value.toFixed(4) : ''}} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.html`
around lines 4 - 6, The template calls data.value.toFixed(4) without guarding
for null/undefined; update the elementlabel.component.html expression (the line
rendering {{data.value.toFixed(4)}}) to guard against missing values—either use
Angular safe navigation/optional chaining and a default (e.g.,
{{data?.value?.toFixed(4) ?? ''}} or {{data.value?.toFixed(4) ?? ''}} if
allowed) or wrap the display in an *ngIf that checks data && data.value !==
undefined; also ensure the component's `@Input`() data is appropriately typed
(e.g., DataPoint | undefined) in the ElementLabelComponent so the template and
type system reflect the possible absence of value.
Comment on lines
+19
to
+21
| get fontSize() { | ||
| return `${20 / this.lines}px`; | ||
| } |
Contributor
There was a problem hiding this comment.
Division by zero when lines is 0 produces invalid CSS.
The template passes lines=0 for non-operator types (d.type.name === 'operator' ? getLines(yFactor) : 0). When lines is 0, fontSize returns "Infinitypx", which is invalid CSS.
🐛 Proposed fix
get fontSize() {
+ if (!this.lines || this.lines <= 0) {
+ return '20px'; // Default font size
+ }
return `${20 / this.lines}px`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| get fontSize() { | |
| return `${20 / this.lines}px`; | |
| } | |
| get fontSize() { | |
| if (!this.lines || this.lines <= 0) { | |
| return '20px'; // Default font size | |
| } | |
| return `${20 / this.lines}px`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@gui-js/libs/ui-components/src/lib/svg-canvas/components/elementlabel.component.ts`
around lines 19 - 21, The fontSize getter can divide by zero when this.lines ===
0; modify the fontSize logic in the elementlabel.component (the fontSize getter)
to guard against zero/negative values (e.g. use Math.max(this.lines, 1) or
return a sensible default like '20px' when this.lines is falsy) so it never
returns "Infinitypx" and always produces valid CSS.
Comment on lines
+149
to
+158
| ngAfterViewInit() { | ||
| this.setDimensions(); | ||
|
|
||
| /*this.rectCollideForce = this.rectCollide(); | ||
| this.rectCollideForce.strength(3); | ||
|
|
||
| this.simulation = d3.forceSimulation() | ||
| .force('rectcollide', this.rectCollideForce) | ||
| .nodes(this.unparkedData);*/ | ||
| } |
Contributor
There was a problem hiding this comment.
rectCollideForce never gets initialized in the current build.
This entire setup is commented out, but the drag handlers still call this.rectCollideForce.sizeModifier(...) later in the file. The first drag will throw before anything moves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.ts` around
lines 149 - 158, The commented-out initialization causes rectCollideForce to be
undefined and later calls to this.rectCollideForce.sizeModifier(...) from the
drag handlers to throw; restore or safely initialize it in ngAfterViewInit by
creating the rectCollide force (call this.rectCollide() to assign
this.rectCollideForce), configure its strength (e.g., strength(3)) and, if
required, reattach it to the simulation (this.simulation =
d3.forceSimulation().force('rectcollide',
this.rectCollideForce).nodes(this.unparkedData)); alternatively, add a defensive
guard in the drag handlers (check if this.rectCollideForce exists before calling
sizeModifier) so the first drag won't throw.
Comment on lines
+479
to
+489
| dialogRef.afterClosed().subscribe((result: EditVariableDialogData) => { | ||
| if(result) { | ||
| d.symbol = result.symbol; | ||
| d.name = result.name; | ||
| d.description = result.description; | ||
| if(['constant', 'stock'].includes(d.type.name)) { | ||
| d.value = result.value; | ||
| d.minimum = result.minimum; | ||
| d.maximum = result.maximum; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Recompute derived geometry after dialog edits.
SvgCanvasHelper.createData() derives connectors, actualLength, and dimensions from symbol/type, but this save path only mutates raw fields. After a rename, the node width and connector geometry still reflect the old symbol.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.ts` around
lines 479 - 489, The dialog close handler currently updates only raw fields on
the node variable d (symbol, name, description, value/minimum/maximum) but does
not recompute derived geometry, so connector geometry, actualLength and
dimensions remain stale; after mutating d inside the
dialogRef.afterClosed().subscribe callback, call the geometry recomputation used
elsewhere (e.g., SvgCanvasHelper.createData(d) or the component method that
wraps it) and merge/assign its returned connectors, actualLength and dimensions
back onto d (or replace d with the returned data) so the node width and
connector geometry update immediately after edits.
Comment on lines
+494
to
+498
| getConnectorPos(c) { | ||
| const points = c.position(0,0,(c.data.type.boundingbox[2] / 2), (c.data.type.boundingbox[3] / 2)); | ||
| const rotated = this.rotatePoint(points, c.data.rotation); | ||
| return [rotated[0] + c.data.x, rotated[1] + c.data.y, points[2] + c.data.rotation]; | ||
| } |
Contributor
There was a problem hiding this comment.
Use dimensions.boundingbox for connector placement.
SvgVariableTypes does not expose a boundingbox field on type; SvgCanvasHelper.createData() stores it on d.dimensions. c.data.type.boundingbox is therefore undefined here, so showing connectors will blow up when this code runs.
Suggested fix
- const points = c.position(0,0,(c.data.type.boundingbox[2] / 2), (c.data.type.boundingbox[3] / 2));
+ const points = c.position(
+ 0,
+ 0,
+ c.data.dimensions.boundingbox[2] / 2,
+ c.data.dimensions.boundingbox[3] / 2
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| getConnectorPos(c) { | |
| const points = c.position(0,0,(c.data.type.boundingbox[2] / 2), (c.data.type.boundingbox[3] / 2)); | |
| const rotated = this.rotatePoint(points, c.data.rotation); | |
| return [rotated[0] + c.data.x, rotated[1] + c.data.y, points[2] + c.data.rotation]; | |
| } | |
| getConnectorPos(c) { | |
| const points = c.position( | |
| 0, | |
| 0, | |
| c.data.dimensions.boundingbox[2] / 2, | |
| c.data.dimensions.boundingbox[3] / 2 | |
| ); | |
| const rotated = this.rotatePoint(points, c.data.rotation); | |
| return [rotated[0] + c.data.x, rotated[1] + c.data.y, points[2] + c.data.rotation]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/svg-canvas/svg-canvas.component.ts` around
lines 494 - 498, getConnectorPos is using c.data.type.boundingbox which is
undefined because createData stores bounding box under c.data.dimensions; update
getConnectorPos to read the bounding box from c.data.dimensions (e.g. use
c.data.dimensions.boundingbox[2] and [3] when computing points), keep the
subsequent rotatePoint(...) and return shape the same, and ensure callers of
getConnectorPos still receive the same [x,y,rotation] tuple.
Comment on lines
+130
to
+163
| setPositions() { | ||
| const svgData = this.allSvgData.filter(d => { | ||
| return document.forms['variablePane']['variablePane::'+d.type.name].checked; | ||
| }); | ||
| svgData.sort((a,b) => { | ||
| if(a.type.name > b.type.name) return 1; | ||
| if(a.type.name < b.type.name) return -1; | ||
| if(a.symbol > b.symbol) return 1; | ||
| if(a.symbol < b.symbol) return -1; | ||
| return 0; | ||
| }); | ||
|
|
||
| const columnLength = Math.round(this.variablePaneContainer.clientHeight / 50); | ||
| const numberOfColumns = svgData.length / columnLength; | ||
| let currentXOffset = 10; | ||
| for(let i = 0; i < numberOfColumns; i++) { | ||
| let maxX = 0; | ||
| for(let j = 0; j < columnLength; j++) { | ||
| const d = svgData[i * columnLength + j]; | ||
| if(!d) break; | ||
| maxX = Math.max(maxX, d.dimensions.boundingbox[2]); | ||
| } | ||
| for(let j = 0; j < columnLength; j++) { | ||
| const d = svgData[i * columnLength + j]; | ||
| if(!d) break; | ||
| d.x = currentXOffset - d.dimensions.boundingbox[0]; | ||
| d.y = 40 + 60 * j; | ||
| } | ||
| currentXOffset += maxX + 20; | ||
| } | ||
| if (event.code==='F1') | ||
| this.electronService.send(events.HELP_FOR, {classType:'VariableBrowser'}); | ||
| }; | ||
| onKeyUp = (event: KeyboardEvent) => { | ||
| document.body.style.cursor='default'; | ||
| this.variablePane.shift(false); | ||
| this.variablePane.requestRedraw(); | ||
| }; | ||
| this.containerWidth$.next(`${currentXOffset}px`); | ||
| this.svgData$.next(svgData); | ||
| this.svgCanvas.setDimensions(currentXOffset); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard setPositions() until initialization completes.
This method is called from the resize listener, but initEvents() populates allSvgData and svgCanvas asynchronously. A resize before that finishes will hit this.allSvgData.filter(...) on undefined.
Suggested fix
setPositions() {
+ if (!this.allSvgData || !this.variablePaneContainer || !this.svgCanvas) return;
const svgData = this.allSvgData.filter(d => {
return document.forms['variablePane']['variablePane::'+d.type.name].checked;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setPositions() { | |
| const svgData = this.allSvgData.filter(d => { | |
| return document.forms['variablePane']['variablePane::'+d.type.name].checked; | |
| }); | |
| svgData.sort((a,b) => { | |
| if(a.type.name > b.type.name) return 1; | |
| if(a.type.name < b.type.name) return -1; | |
| if(a.symbol > b.symbol) return 1; | |
| if(a.symbol < b.symbol) return -1; | |
| return 0; | |
| }); | |
| const columnLength = Math.round(this.variablePaneContainer.clientHeight / 50); | |
| const numberOfColumns = svgData.length / columnLength; | |
| let currentXOffset = 10; | |
| for(let i = 0; i < numberOfColumns; i++) { | |
| let maxX = 0; | |
| for(let j = 0; j < columnLength; j++) { | |
| const d = svgData[i * columnLength + j]; | |
| if(!d) break; | |
| maxX = Math.max(maxX, d.dimensions.boundingbox[2]); | |
| } | |
| for(let j = 0; j < columnLength; j++) { | |
| const d = svgData[i * columnLength + j]; | |
| if(!d) break; | |
| d.x = currentXOffset - d.dimensions.boundingbox[0]; | |
| d.y = 40 + 60 * j; | |
| } | |
| currentXOffset += maxX + 20; | |
| } | |
| if (event.code==='F1') | |
| this.electronService.send(events.HELP_FOR, {classType:'VariableBrowser'}); | |
| }; | |
| onKeyUp = (event: KeyboardEvent) => { | |
| document.body.style.cursor='default'; | |
| this.variablePane.shift(false); | |
| this.variablePane.requestRedraw(); | |
| }; | |
| this.containerWidth$.next(`${currentXOffset}px`); | |
| this.svgData$.next(svgData); | |
| this.svgCanvas.setDimensions(currentXOffset); | |
| } | |
| setPositions() { | |
| if (!this.allSvgData || !this.variablePaneContainer || !this.svgCanvas) return; | |
| const svgData = this.allSvgData.filter(d => { | |
| return document.forms['variablePane']['variablePane::'+d.type.name].checked; | |
| }); | |
| svgData.sort((a,b) => { | |
| if(a.type.name > b.type.name) return 1; | |
| if(a.type.name < b.type.name) return -1; | |
| if(a.symbol > b.symbol) return 1; | |
| if(a.symbol < b.symbol) return -1; | |
| return 0; | |
| }); | |
| const columnLength = Math.round(this.variablePaneContainer.clientHeight / 50); | |
| const numberOfColumns = svgData.length / columnLength; | |
| let currentXOffset = 10; | |
| for(let i = 0; i < numberOfColumns; i++) { | |
| let maxX = 0; | |
| for(let j = 0; j < columnLength; j++) { | |
| const d = svgData[i * columnLength + j]; | |
| if(!d) break; | |
| maxX = Math.max(maxX, d.dimensions.boundingbox[2]); | |
| } | |
| for(let j = 0; j < columnLength; j++) { | |
| const d = svgData[i * columnLength + j]; | |
| if(!d) break; | |
| d.x = currentXOffset - d.dimensions.boundingbox[0]; | |
| d.y = 40 + 60 * j; | |
| } | |
| currentXOffset += maxX + 20; | |
| } | |
| this.containerWidth$.next(`${currentXOffset}px`); | |
| this.svgData$.next(svgData); | |
| this.svgCanvas.setDimensions(currentXOffset); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts`
around lines 130 - 163, Guard the setPositions() method against running before
initialization by returning early when required fields are not ready: check that
this.allSvgData is defined and is an array, this.svgCanvas is initialized, and
this.variablePaneContainer (and the form elements used) exist before proceeding;
if any are absent, simply return. Update setPositions() (referencing
setPositions, allSvgData, svgCanvas, variablePaneContainer, containerWidth$,
svgData$, svgCanvas.setDimensions) to perform these null/undefined checks at the
top so the resize listener can safely call it before initEvents() completes.
Comment on lines
+142
to
+145
| const columnLength = Math.round(this.variablePaneContainer.clientHeight / 50); | ||
| const numberOfColumns = svgData.length / columnLength; | ||
| let currentXOffset = 10; | ||
| for(let i = 0; i < numberOfColumns; i++) { |
Contributor
There was a problem hiding this comment.
Clamp columnLength before dividing by it.
Math.round(this.variablePaneContainer.clientHeight / 50) can be 0 while the pane is collapsed or not measured yet. That makes numberOfColumns infinite and the outer for loop unbounded.
Suggested fix
- const columnLength = Math.round(this.variablePaneContainer.clientHeight / 50);
- const numberOfColumns = svgData.length / columnLength;
+ const columnLength = Math.max(1, Math.floor(this.variablePaneContainer.clientHeight / 50));
+ const numberOfColumns = Math.ceil(svgData.length / columnLength);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const columnLength = Math.round(this.variablePaneContainer.clientHeight / 50); | |
| const numberOfColumns = svgData.length / columnLength; | |
| let currentXOffset = 10; | |
| for(let i = 0; i < numberOfColumns; i++) { | |
| const columnLength = Math.max(1, Math.floor(this.variablePaneContainer.clientHeight / 50)); | |
| const numberOfColumns = Math.ceil(svgData.length / columnLength); | |
| let currentXOffset = 10; | |
| for(let i = 0; i < numberOfColumns; i++) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts`
around lines 142 - 145, Clamp the computed columnLength to a minimum of 1 before
using it to compute numberOfColumns to avoid division-by-zero/infinite loops:
replace the raw Math.round(this.variablePaneContainer.clientHeight / 50) with
something like const columnLength = Math.max(1,
Math.round(this.variablePaneContainer.clientHeight / 50)); then compute
numberOfColumns using svgData.length and columnLength (and consider using
Math.ceil or Math.floor as appropriate) so the for loop over numberOfColumns
(referencing variablePaneContainer, columnLength, numberOfColumns, svgData) is
always bounded.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
gui-js/libs/ui-components/src/lib/svg-canvas/constants/svg-constants.ts (3)
3-141: Consider consolidating duplicate type definitions.
constantandparameterhave identicalpoints,getDimensions,stroke, andclassdefinitions. Similarly,stockandintegralare identical. This duplication increases maintenance burden and risk of divergence.Example approach to reduce duplication
// Define shared base configurations const pentagonShape = { points: (x: number, y: number, width: number, height: number) => `${x-width},${y-height} ${x+width*0.75},${y-height} ${x+width},${y} ${x+width*0.75},${y+height} ${x-width},${y+height}`, getDimensions: (length: number) => { const lengthPart = length / 12; return { length, boundingbox: [-4 * lengthPart, -25, 8 * lengthPart, 50], labelbox: [-4 * lengthPart, -22.5, 6 * lengthPart, 45], valuebox: [lengthPart * 1, -20, lengthPart * 6, 25], exponentbox: [lengthPart * 1, 5, lengthPart * 6, 25], valueline: [[-4 * lengthPart, -25], [3 * lengthPart, -25]] }; } }; // Then extend for specific types public static constant = { ...pentagonShape, name: 'constant', connectors: [{ type: 'out', position: ... }], stroke: '#1c19d3', class: { 'constant': true } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/constants/svg-constants.ts` around lines 3 - 141, SvgVariableTypes has duplicated configuration for shapes (notably SvgVariableTypes.constant and SvgVariableTypes.parameter, and SvgVariableTypes.stock and SvgVariableTypes.integral); extract the shared pieces (points, getDimensions, stroke, class) into reusable constants or factory helpers (e.g., a pentagonShape and pentagonVariableShape or a makeShape(name, connectors, overrides) factory) and then replace the duplicated inline definitions in SvgVariableTypes.constant, .parameter, .stock and .integral with spreads or calls to the factory to compose the final object, preserving unique fields like name and connectors.
176-196: Add type annotations for better type safety.The
latexparameter lacks a type annotation, defaulting to implicitany. Adding explicit types improves maintainability and catches type mismatches at compile time.Proposed type annotations
- static estimateVisibleLatex(latex) { + static estimateVisibleLatex(latex: string): number { let clean = latex;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/constants/svg-constants.ts` around lines 176 - 196, The estimateVisibleLatex function is missing TypeScript type annotations for its parameter and return value; update static estimateVisibleLatex to accept a typed parameter (e.g., latex: string) and declare an explicit return type (e.g., : number), and if needed guard against null/undefined inputs by coalescing or validating the string at the start of the function (e.g., treat non-strings as '' or throw) so the internal string operations are type-safe; reference the static method estimateVisibleLatex to locate and apply this change.
198-204: Add type annotations forcreateData.The
templateparameter and return type lack annotations. Given this method is used across multiple components (variable-pane, svg-canvas), explicit typing would improve IDE support and catch misuse early.Proposed type annotations
- static createData(template) { + static createData(template: { symbol: string; type: { connectors: any[]; getDimensions: (length: number) => any }; [key: string]: any }) { const d = Object.assign({}, template); d.connectors = d.type.connectors.map(c => Object.assign({},c,{data: d})); d.actualLength = Math.max(100, SvgCanvasHelper.estimateVisibleLatex(template.symbol) * 15) + 120; d.dimensions = d.type.getDimensions(d.actualLength); return d; }Alternatively, consider defining a proper interface (e.g.,
DataPointTemplate) for the input and aDataPointinterface for the return type to fully leverage TypeScript's type system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/svg-canvas/constants/svg-constants.ts` around lines 198 - 204, The createData static method is missing TypeScript type annotations; add explicit types for the parameter and return value (e.g., define an input interface like DataPointTemplate and a return interface like DataPoint) and update the signature of createData(template: DataPointTemplate): DataPoint; ensure the interfaces include properties used inside the method such as type (with connectors and getDimensions), symbol, connectors array items (with whatever properties c has), actualLength, dimensions, and that connectors mapping (d.type.connectors.map(c => Object.assign({}, c, { data: d }))) types reflect the added data back-reference so the compiler knows c can receive a data field; update any exported types so callers in variable-pane and svg-canvas pick up the new definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gui-js/libs/ui-components/src/lib/svg-canvas/constants/svg-constants.ts`:
- Around line 3-141: SvgVariableTypes has duplicated configuration for shapes
(notably SvgVariableTypes.constant and SvgVariableTypes.parameter, and
SvgVariableTypes.stock and SvgVariableTypes.integral); extract the shared pieces
(points, getDimensions, stroke, class) into reusable constants or factory
helpers (e.g., a pentagonShape and pentagonVariableShape or a makeShape(name,
connectors, overrides) factory) and then replace the duplicated inline
definitions in SvgVariableTypes.constant, .parameter, .stock and .integral with
spreads or calls to the factory to compose the final object, preserving unique
fields like name and connectors.
- Around line 176-196: The estimateVisibleLatex function is missing TypeScript
type annotations for its parameter and return value; update static
estimateVisibleLatex to accept a typed parameter (e.g., latex: string) and
declare an explicit return type (e.g., : number), and if needed guard against
null/undefined inputs by coalescing or validating the string at the start of the
function (e.g., treat non-strings as '' or throw) so the internal string
operations are type-safe; reference the static method estimateVisibleLatex to
locate and apply this change.
- Around line 198-204: The createData static method is missing TypeScript type
annotations; add explicit types for the parameter and return value (e.g., define
an input interface like DataPointTemplate and a return interface like DataPoint)
and update the signature of createData(template: DataPointTemplate): DataPoint;
ensure the interfaces include properties used inside the method such as type
(with connectors and getDimensions), symbol, connectors array items (with
whatever properties c has), actualLength, dimensions, and that connectors
mapping (d.type.connectors.map(c => Object.assign({}, c, { data: d }))) types
reflect the added data back-reference so the compiler knows c can receive a data
field; update any exported types so callers in variable-pane and svg-canvas pick
up the new definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3edb498-215e-409d-b7d4-b6410473fc4d
📒 Files selected for processing (1)
gui-js/libs/ui-components/src/lib/svg-canvas/constants/svg-constants.ts
filter out weird types like "tempFlow" until I know better what to do with them
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts (3)
80-83: Remove unusedwindowResize()method.This method is never called and duplicates the assignment already performed in
initEvents()at lines 86-87.♻️ Proposed fix
- windowResize() { - this.variablePaneContainer = this.variablePaneWrapper - .nativeElement as HTMLElement; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts` around lines 80 - 83, Remove the unused windowResize() method because it duplicates the assignment already done in initEvents(); delete the windowResize() function definition and any unused imports or bindings that reference it, leaving the variablePaneContainer assignment in initEvents() (using variablePaneWrapper.nativeElement) as the single source of truth; ensure no event listeners or templates still refer to windowResize before committing.
89-93: Consider cleaning up the wheel event listener on destroy.The wheel event listener is added imperatively but not removed in
ngOnDestroy(). While Angular typically cleans up DOM elements on component destruction, explicitly removing the listener is safer and aligns with the existing cleanup pattern usingdestroy$.♻️ Suggested approach
Store a reference to the handler and remove it on destroy:
private wheelHandler = (evt: WheelEvent) => { evt.preventDefault(); this.variablePaneContainer.scrollLeft += evt.deltaY; }; // In initEvents(): this.variablePaneContainer.addEventListener('wheel', this.wheelHandler, { passive: false }); // In ngOnDestroy(): this.variablePaneContainer?.removeEventListener('wheel', this.wheelHandler);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts` around lines 89 - 93, The wheel event listener added to variablePaneContainer in initEvents is not removed on destroy; define a named handler (e.g., private wheelHandler = (evt: WheelEvent) => { evt.preventDefault(); this.variablePaneContainer.scrollLeft += evt.deltaY; }), use it when adding the listener (this.variablePaneContainer.addEventListener('wheel', this.wheelHandler, { passive: false })), and remove it in ngOnDestroy by calling this.variablePaneContainer?.removeEventListener('wheel', this.wheelHandler) to mirror the existing destroy$ cleanup pattern.
111-112: Unreachable code:'constant'is already filtered out.The check for
'constant'at line 111 is unreachable because line 109 already continues past any type not insupportedTypes(which does not include'constant').♻️ Proposed cleanup
if(!supportedTypes.some(t => t === typeName)) continue; - let trimmedName; - if(typeName === 'constant') { - continue; - } else { - trimmedName = (await varValue.name()); + let trimmedName = await varValue.name(); - const splitName = trimmedName.split(':'); - if(splitName.length > 1) { - trimmedName = splitName[1]; - } + const splitName = trimmedName.split(':'); + if(splitName.length > 1) { + trimmedName = splitName[1]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts` around lines 111 - 112, The if-check for typeName === 'constant' is unreachable because supportedTypes (used in the earlier filter/continue) already excludes 'constant'; remove the redundant block inside the loop in variable-pane.component.ts (the if (typeName === 'constant') { continue; } check) so the loop relies solely on the supportedTypes filter and avoid dead code in the function that iterates variables using typeName and supportedTypes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts`:
- Around line 125-126: The DataPoint class is missing the optional numeric
properties minimum and maximum referenced when assigning d.minimum = await
varValue.sliderMin() and d.maximum = await varValue.sliderMax(); add "minimum?:
number;" and "maximum?: number;" to the DataPoint class declaration so
TypeScript knows these fields exist; update the DataPoint type used across
components (notably variable-pane.component.ts where d is assigned and
svg-canvas.component.ts methods getRelativeValue()/setRelativeValue() which read
them) to restore type safety and prevent compiler errors.
---
Nitpick comments:
In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts`:
- Around line 80-83: Remove the unused windowResize() method because it
duplicates the assignment already done in initEvents(); delete the
windowResize() function definition and any unused imports or bindings that
reference it, leaving the variablePaneContainer assignment in initEvents()
(using variablePaneWrapper.nativeElement) as the single source of truth; ensure
no event listeners or templates still refer to windowResize before committing.
- Around line 89-93: The wheel event listener added to variablePaneContainer in
initEvents is not removed on destroy; define a named handler (e.g., private
wheelHandler = (evt: WheelEvent) => { evt.preventDefault();
this.variablePaneContainer.scrollLeft += evt.deltaY; }), use it when adding the
listener (this.variablePaneContainer.addEventListener('wheel',
this.wheelHandler, { passive: false })), and remove it in ngOnDestroy by calling
this.variablePaneContainer?.removeEventListener('wheel', this.wheelHandler) to
mirror the existing destroy$ cleanup pattern.
- Around line 111-112: The if-check for typeName === 'constant' is unreachable
because supportedTypes (used in the earlier filter/continue) already excludes
'constant'; remove the redundant block inside the loop in
variable-pane.component.ts (the if (typeName === 'constant') { continue; }
check) so the loop relies solely on the supportedTypes filter and avoid dead
code in the function that iterates variables using typeName and supportedTypes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f3bf981-8aac-4a70-a8d9-86e40f95d43e
📒 Files selected for processing (1)
gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts
Comment on lines
+125
to
+126
| d.minimum = await varValue.sliderMin(); | ||
| d.maximum = await varValue.sliderMax(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check current DataPoint class definition
cat gui-js/libs/ui-components/src/lib/svg-canvas/classes/datapoint.tsRepository: highperformancecoder/minsky
Length of output: 572
🏁 Script executed:
# Check the context around lines 125-126 in variable-pane.component.ts
head -n 140 gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts | tail -n 30Repository: highperformancecoder/minsky
Length of output: 1175
🏁 Script executed:
# Search for how variable d is declared and its type
rg "d\.minimum|d\.maximum" gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts -B 10 -A 5Repository: highperformancecoder/minsky
Length of output: 697
🏁 Script executed:
# Search for minimum and maximum usage in DataPoint or related types across the codebase
rg "minimum|maximum" gui-js/libs/ui-components/src/lib/svg-canvas/classes/ -A 2Repository: highperformancecoder/minsky
Length of output: 53
🏁 Script executed:
# Search for SvgCanvasHelper.createData definition
rg "createData" gui-js/libs/ui-components/src/lib/svg-canvas/ -A 5Repository: highperformancecoder/minsky
Length of output: 2057
🏁 Script executed:
# Check TypeScript strict mode configuration
find . -name "tsconfig*.json" -type f | head -5Repository: highperformancecoder/minsky
Length of output: 315
🏁 Script executed:
# Search for other usages of minimum and maximum properties in the component files
rg "\.minimum|\.maximum" gui-js/libs/ui-components/src/lib/ -B 2 -A 2Repository: highperformancecoder/minsky
Length of output: 6733
🏁 Script executed:
# Check the main tsconfig
cat gui-js/apps/minsky-electron/tsconfig.json | head -30Repository: highperformancecoder/minsky
Length of output: 319
🏁 Script executed:
# Check the base tsconfig
cat gui-js/tsconfig.base.json | grep -A 20 "compilerOptions"Repository: highperformancecoder/minsky
Length of output: 753
Add minimum and maximum properties to the DataPoint class.
The code assigns d.minimum and d.maximum at lines 125-126, but these properties are not declared in the DataPoint class definition. While JavaScript allows dynamic property assignment and the code runs at runtime, this creates type safety issues and impacts maintainability. These properties are also used in multiple other components (e.g., svg-canvas.component.ts in getRelativeValue() and setRelativeValue() methods).
Add the following properties to DataPoint:
minimum?: number;
maximum?: number;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui-js/libs/ui-components/src/lib/variable-pane/variable-pane.component.ts`
around lines 125 - 126, The DataPoint class is missing the optional numeric
properties minimum and maximum referenced when assigning d.minimum = await
varValue.sliderMin() and d.maximum = await varValue.sliderMax(); add "minimum?:
number;" and "maximum?: number;" to the DataPoint class declaration so
TypeScript knows these fields exist; update the DataPoint type used across
components (notably variable-pane.component.ts where d is assigned and
svg-canvas.component.ts methods getRelativeValue()/setRelativeValue() which read
them) to restore type safety and prevent compiler errors.
Owner
|
Cool - just took it for a spin. I see you leveraged d3, which you've been promoting for a while. The main purpose of this dialog is to act as a floating toolbar where you can drag an instance of a variable onto the canvas to avoid having to type out its name again. That's the one thing your implementation doesn't do, but I suspect its a one-line fix. As for the purpose, probably a simple HTML table with variable names rendered using mathjax would do the job, and be much simpler. But whatever makes most sense from a UX point of view. I haven't browsed the code yet, but note that CodeRabbit has some nits that'd be worth addressing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Variable browser that dynamically adjusts the number of variables to be displayed vertically based on the window's height and allows sideways scrolling when the variables fall outside of the window's horizontal dimensions.
This introduces the svg-canvas component that I made as a demo earlier, which uses d3 and svg elements to display variables, parameters and constants.
This change is
Summary by CodeRabbit
New Features
Security
Improvements