Release 2.1.0 #30
Release 2.1.0 #30
Conversation
- start development cycle
- remove questionable *ngIf on Placeholder and Description inputs in edit panel
Initial loading
- refactor data detail to allow different init type inputs
- refactor into angular component
- reuse component in form builder
- add sort for task list
- update monaco petriflow definition, remove regex highlight due to collision with comments
- fix getReferenceValue getting value from original model and not simulated model
[NAB-369] - Boolean field description
[NAB-373] - Place reference value not changed on simulation
[NAB-362] Action editor interpolated string highlight
- add dialog for changing place marking - add validations to existing data change dialog - bind arcs and places in simulation mode - fix typo in place edit dialog
- add missing css for dialog
- fix data dialog opening on arcs with place ref
- refactor dialog call into common functions
- add interface definitions for field list - add new component definitions - add default component properties
- change input in data view to autocomplete, prepare for component refactor
- update components in edit panel to autocomplete (WIP)
- reset tool after closing dialog - clear selected elements collection on reset
[NAB-372] - Element move after dialog save
# Conflicts: # src/app/modeler/edit-mode/services/modes/canvas-tool.ts
- fix merge conflict
- fix resetting simulation on dialog close by refactoring dialog open function
[NAB-367] - One-click variable arc change
- increase RC version
- increase version of petri.svg and petriflow.svg libs
- compare init values by transition id if no label is present
- disable tasks that contain taskRef as dataRef
[NAB-359] Task ref with multiple init values
# Conflicts: # src/app/modeler/data-mode/data-detail/data-detail.component.ts
… arcs not to load - arcs which referenced data has non integer value will be displayed/represented with value 0 in edit + simulation mode
- prefill known component properties with default values in form builder (click + drag&drop) - update fields with placeholder (caseRef/taskRef...) in form builder - fix i18n text field default properties in form builder
[NAB-368] Component parameters
- Add global flag support for roles and improve role ID form - Enhanced role model to include a global flag. - Updated role-detail component to handle global flag changes. - Updated `@netgrif/petriflow` dependency to 2.2.1.
… arcs not to load - Improved usage of ImportUtils.isInitValueNumber to replace regex checks for initialization values.
[NAB-375] Variables with no init values referenced in arcs will cause arcs not to load
[NAB-378] Global role support
… arcs not to load - fix missing init value check
- increase version rc number
- fix init value not set in data detail component
- increase version rc number
- fix duplicate action id bug
- increase version rc number
- Update `@netgrif/petriflow` dependency to 2.2.2.
- fix the problem with columns, add datagroup if there is no one and set it number of cols
WalkthroughAdds dialog-based editing for place markings and data in simulation, extensive form-builder autocompletes and typed field metadata, ModelService action ID management, a tokenizer rewrite, UI loading styles, package/config updates, and multiple component/service integrations and refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SimulationTool
participant CanvasPlace
participant DialogMarkingChangeComponent
participant ModelService
participant SimulationModeService
User->>SimulationTool: click place (onPlaceUp)
SimulationTool->>CanvasPlace: resolve place id
SimulationTool->>DialogMarkingChangeComponent: openDialog(placeId, callback)
DialogMarkingChangeComponent->>ModelService: read place
ModelService-->>DialogMarkingChangeComponent: place object
User->>DialogMarkingChangeComponent: submit new marking
DialogMarkingChangeComponent-->>SimulationTool: callback(updated marking)
SimulationTool->>ModelService: apply marking
SimulationTool->>SimulationModeService: renderModel(...)
SimulationModeService-->>User: updated visualization
sequenceDiagram
participant User
participant SimulationTool
participant CanvasArc
participant DialogChangeDataComponent
participant SimulationModeService
participant ModelService
User->>SimulationTool: click arc (onArcUp)
alt arc references data
SimulationTool->>DialogChangeDataComponent: openDialog(dataSet, callback)
User->>DialogChangeDataComponent: edit & save
DialogChangeDataComponent-->>SimulationTool: callback(updated data)
SimulationTool->>ModelService: update data model
SimulationTool->>SimulationModeService: renderModel(...)
else arc references place
SimulationTool->>DialogMarkingChangeComponent: openDialog(placeId, callback)
User->>DialogMarkingChangeComponent: edit & save
DialogMarkingChangeComponent-->>SimulationTool: callback(updated marking)
SimulationTool->>ModelService: apply marking
SimulationTool->>SimulationModeService: renderModel(...)
end
SimulationModeService-->>User: updated visualization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (44)
.gitignore(1 hunks)nae.json(1 hunks)package.json(2 hunks)src/app/app.module.ts(2 hunks)src/app/dialogs/dialog-change-data/dialog-change-data.component.html(1 hunks)src/app/dialogs/dialog-change-data/dialog-change-data.component.ts(2 hunks)src/app/dialogs/dialog-marking-change/dialog-marking-change.component.html(1 hunks)src/app/dialogs/dialog-marking-change/dialog-marking-change.component.scss(1 hunks)src/app/dialogs/dialog-marking-change/dialog-marking-change.component.ts(1 hunks)src/app/dialogs/dialog-place-edit/dialog-place-edit.component.html(1 hunks)src/app/form-builder/edit-panel/edit-panel.component.html(7 hunks)src/app/form-builder/edit-panel/edit-panel.component.ts(6 hunks)src/app/form-builder/field-list/field-list.component.ts(4 hunks)src/app/form-builder/field-list/field-list.service.ts(9 hunks)src/app/form-builder/form-builder.component.html(1 hunks)src/app/form-builder/form-builder.module.ts(2 hunks)src/app/form-builder/gridster/gridster-datafield/gridster-data-field.component.html(1 hunks)src/app/form-builder/gridster/gridster-datafield/gridster-data-field.component.ts(2 hunks)src/app/form-builder/gridster/gridster.service.ts(3 hunks)src/app/modeler/actions-mode/action-editor/action-editor-menu/action-editor-menu.component.ts(1 hunks)src/app/modeler/actions-mode/action-editor/action-editor.service.ts(2 hunks)src/app/modeler/actions-mode/action-editor/definitions/tokens.ts(1 hunks)src/app/modeler/data-mode/data-detail/data-detail.component.html(4 hunks)src/app/modeler/data-mode/data-detail/data-detail.component.ts(9 hunks)src/app/modeler/data-mode/task-ref-init-field/task-ref-init-field.component.html(1 hunks)src/app/modeler/data-mode/task-ref-init-field/task-ref-init-field.component.scss(1 hunks)src/app/modeler/data-mode/task-ref-init-field/task-ref-init-field.component.spec.ts(1 hunks)src/app/modeler/data-mode/task-ref-init-field/task-ref-init-field.component.ts(1 hunks)src/app/modeler/edit-mode/domain/canvas-node-element.ts(2 hunks)src/app/modeler/edit-mode/edit-mode.component.ts(2 hunks)src/app/modeler/edit-mode/services/modes/canvas-tool.ts(0 hunks)src/app/modeler/edit-mode/services/modes/select-tool.ts(1 hunks)src/app/modeler/modeler.module.ts(2 hunks)src/app/modeler/role-mode/role-detail/role-detail.component.html(2 hunks)src/app/modeler/role-mode/role-detail/role-detail.component.ts(3 hunks)src/app/modeler/services/canvas/canvas-listener-tool.ts(1 hunks)src/app/modeler/services/model/model.service.ts(13 hunks)src/app/modeler/services/model/sequence-generator.ts(2 hunks)src/app/modeler/simulation-mode/simulation-mode.component.ts(2 hunks)src/app/modeler/simulation-mode/simulation-mode.service.ts(5 hunks)src/app/modeler/simulation-mode/tool/change-data-tool.ts(1 hunks)src/app/modeler/simulation-mode/tool/simulation-tool.ts(4 hunks)src/index.html(1 hunks)src/styles.scss(1 hunks)
💤 Files with no reviewable changes (1)
- src/app/modeler/edit-mode/services/modes/canvas-tool.ts
🧰 Additional context used
🧬 Code graph analysis (9)
src/app/form-builder/field-list/field-list.component.ts (2)
src/app/form-builder/field-list/field-list.service.ts (1)
ComponentDef(11-18)src/app/form-builder/edit-panel/edit-panel.component.ts (1)
dataRef(182-184)
src/app/modeler/simulation-mode/simulation-mode.service.ts (2)
src/app/modeler/services/model/model.service.ts (3)
Injectable(33-597)model(73-82)model(84-86)src/app/modeler/edit-mode/edit-mode.service.ts (1)
Injectable(45-367)
src/app/dialogs/dialog-marking-change/dialog-marking-change.component.ts (3)
src/app/dialogs/dialog-change-data/dialog-change-data.component.ts (1)
Component(16-46)src/app/modeler/simulation-mode/simulation-mode.service.ts (2)
data(149-151)data(153-155)src/app/dialogs/dialog-place-edit/dialog-place-edit.component.ts (1)
PlaceEditData(7-9)
src/app/modeler/role-mode/role-detail/role-detail.component.ts (1)
src/app/modeler/role-mode/role-detail/changed-role.ts (1)
ChangedRole(3-14)
src/app/dialogs/dialog-change-data/dialog-change-data.component.ts (2)
src/app/modeler/simulation-mode/simulation-mode.service.ts (2)
data(149-151)data(153-155)src/app/modeler/edit-mode/domain/canvas-node-element.ts (1)
id(7-9)
src/app/modeler/simulation-mode/tool/change-data-tool.ts (2)
src/app/dialogs/dialog-change-data/dialog-change-data.component.ts (2)
DataSet(7-9)Data(11-14)src/app/modeler/simulation-mode/simulation-mode.service.ts (2)
data(149-151)data(153-155)
src/app/modeler/data-mode/data-detail/data-detail.component.ts (1)
src/app/form-builder/field-list/field-list.service.ts (3)
ComponentDef(11-18)DataRefDef(20-25)PropertyDef(6-9)
src/app/form-builder/gridster/gridster.service.ts (1)
src/app/form-builder/field-list/field-list.service.ts (1)
PropertyDef(6-9)
src/app/form-builder/edit-panel/edit-panel.component.ts (1)
src/app/form-builder/field-list/field-list.service.ts (3)
ComponentDef(11-18)DataRefDef(20-25)PropertyDef(6-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build and test
🔇 Additional comments (51)
src/app/modeler/role-mode/role-detail/role-detail.component.html (2)
26-32: LGTM!The "Is Global" checkbox is properly implemented with correct data binding and change handler. The layout structure is consistent with other form fields in the component.
36-36: LGTM!Good formatting improvement - adding spaces inside interpolation braces improves readability and follows Angular style guidelines.
src/app/modeler/role-mode/role-detail/role-detail.component.ts (1)
16-16: LGTM!The addition of trailing commas and the rename from
formtoroleIdFormare good improvements. The trailing commas follow TypeScript best practices and make future diffs cleaner. The more descriptive form control name improves code readability.Also applies to: 30-30, 39-42
src/index.html (3)
5-5: LGTM!The title capitalization improvement enhances the professional appearance of the application.
13-69: LGTM!The inline spinner styles are appropriate for displaying the loading screen immediately before the main CSS bundle loads. This approach ensures users see visual feedback during application startup.
75-80: LGTM!The loading layout structure is well-designed, with the logo placeholder and spinner providing clear visual feedback during application initialization. The integration with the styles defined in
styles.scssensures a smooth loading experience.src/styles.scss (1)
233-259: LGTM!The loading block and fade-in animation are implemented correctly. The technique of switching
displayfromnonetoblockat 1% of the animation, followed by opacity animation, is a valid workaround for the fact that thedisplayproperty cannot be smoothly animated. The referenced asset file exists atsrc/assets/netgrif_logo.svg.src/app/modeler/data-mode/task-ref-init-field/task-ref-init-field.component.scss (1)
1-3: LGTM!The utility class is straightforward and follows standard CSS patterns for full-width layouts.
.gitignore (1)
54-54: LGTM!Adding pnpm-lock.yaml to the ignore list is consistent with the existing pattern of ignoring package-lock.json and is appropriate for this project.
src/app/dialogs/dialog-change-data/dialog-change-data.component.html (1)
1-1: LGTM!The dynamic plural/singular title based on dataset length improves the user experience.
src/app/modeler/actions-mode/action-editor/action-editor-menu/action-editor-menu.component.ts (1)
39-41: LGTM!The conditional newline insertion prevents appending unnecessary newlines to falsy values, which improves the text insertion behavior for edge cases.
src/app/modeler/data-mode/task-ref-init-field/task-ref-init-field.component.html (1)
1-26: LGTM!The template correctly implements a chip-based input with autocomplete functionality. The use of Angular 17+ control flow syntax (@for) and Material components is appropriate. Event handlers are properly bound for adding, selecting, and removing init values.
src/app/dialogs/dialog-marking-change/dialog-marking-change.component.scss (1)
1-8: LGTM!The dialog styling follows Material Design patterns with appropriate flex layout for content and right-aligned action buttons.
package.json (1)
61-63: All specified @netgrif package versions are published and available.Verified that @netgrif/petri.svg@1.1.1, @netgrif/petriflow@2.2.2, and @netgrif/petriflow.svg@1.1.1 all exist on npm. No vulnerabilities detected.
src/app/form-builder/form-builder.module.ts (1)
18-18: LGTM! Clean module integration.The ModelerModule import follows Angular best practices and enables FormBuilderModule to access modeler components.
Also applies to: 43-44
src/app/dialogs/dialog-place-edit/dialog-place-edit.component.html (1)
16-16: LGTM! Corrects misleading validation message.The error message now accurately reflects that it validates the Tokens field, not the Id field.
src/app/modeler/simulation-mode/simulation-mode.component.ts (1)
1-1: LGTM! Standard context menu suppression.The right-click handler correctly prevents the default browser context menu, likely to enable custom context menu functionality in simulation mode.
Also applies to: 23-27
src/app/modeler/edit-mode/edit-mode.component.ts (1)
1-1: LGTM! Consistent context menu handling.The right-click handler implementation is identical to SimulationModeComponent, ensuring consistent UX across mode components.
Also applies to: 31-35
src/app/app.module.ts (1)
47-47: LGTM! Proper component registration.DialogMarkingChangeComponent is correctly imported and declared in AppModule, following Angular best practices.
Also applies to: 81-81
src/app/form-builder/gridster/gridster-datafield/gridster-data-field.component.ts (1)
9-9: LGTM! Clean service injection.FieldListService is properly injected as a public property, making it accessible in the component template for placeholder field determination.
Also applies to: 24-26
src/app/dialogs/dialog-marking-change/dialog-marking-change.component.html (1)
7-7: No action needed. The different validator names (validMarkingin dialog-place-edit andvalidMultiplicityin dialog-marking-change) are intentionally named based on their semantic context—validMarking for marking fields and validMultiplicity for count/multiplicity fields. Both implement the same non-negative integer validation logic, but the naming distinction appropriately reflects the field types being validated, which is consistent with best practices.src/app/modeler/edit-mode/domain/canvas-node-element.ts (1)
23-27: LGTM!The
pretty()helper method provides a clean, consistent way to format node labels for UI display, safely handling cases wherelabel.valueis missing.src/app/modeler/services/model/sequence-generator.ts (2)
18-18: Good fix: Using dynamic prefix length.The change from a hardcoded substring index to
this._prefix.lengthcorrectly handles prefixes of any length, fixing a potential bug with non-single-character prefixes.
16-16: LGTM: Action type added to reset() signature.Extending the collection type to include
Actionaligns with the broader ID management enhancements and maintains type safety.src/app/modeler/simulation-mode/simulation-mode.service.ts (3)
62-62: LGTM: Model-aware reference resolution.Passing
this.modeltogetReferenceValue()ensures the multiplicity calculation uses the correct model context.
90-96: Good improvement: Using ImportUtils for robust parsing.Replacing direct parsing with
ImportUtils.isInitValueNumber()provides more robust handling of initialization values and explicit fallback to 0 for non-numeric values.
125-135: LGTM: Consistent tool binding pattern.The
newSvgPlace()andnewSvgArc()methods follow the established pattern from edit-mode (binding created elements to the active tool) and maintain consistency across canvas modes.src/app/form-builder/field-list/field-list.component.ts (2)
98-100: LGTM: Property metadata serialization.Serializing
meta.propertiesas JSON enables property metadata to be transferred during drag-and-drop operations, supporting the richer component configuration.
110-117: LGTM: Consistent string coercion and property transfer.The string coercion for
rowsandcolsensures consistent data transfer format, and property serialization maintains parity with the new field handler.src/app/modeler/actions-mode/action-editor/action-editor.service.ts (1)
39-48: Good refactoring: Centralizing ID generation in ModelService.Delegating action ID generation to
ModelService.nextActionId()follows the single responsibility principle and reduces code duplication. Ensure thatModelService.nextActionId()is properly implemented and maintains ID uniqueness across the application.#!/bin/bash # Description: Verify ModelService.nextActionId() implementation exists and is used correctly # Check for nextActionId implementation in ModelService ast-grep --pattern $'class ModelService { $$$ nextActionId() { $$$ } $$$ }' # Verify all calls to nextActionId go through ModelService rg -n -C3 --type=ts 'nextActionId'src/app/modeler/modeler.module.ts (1)
75-75: Remove unused FormBuilderModule import statement.FormBuilderModule was removed from the @NgModule imports array and is no longer used. Since ModelerModule doesn't depend on FormBuilderComponent and already imports all necessary base modules (GridsterModule, FormsModule, ReactiveFormsModule, etc.), the import statement at line 23 should be removed as cleanup.
src/app/modeler/services/canvas/canvas-listener-tool.ts (1)
342-359: LGTM! Clean dialog lifecycle pattern.The introduction of
beforeDialog()andafterDialog()hooks properly manages keyboard bindings around dialog interactions. The optionalafterClosecallback provides flexibility while ensuring cleanup always occurs viaafterDialog().src/app/modeler/simulation-mode/tool/change-data-tool.ts (1)
38-54: LGTM! Consistent use of the newopenDialogpattern.The refactored code properly leverages the parent class's
openDialogmethod with the callback pattern, ensuring keyboard bindings are correctly managed during dialog interaction. The null check ondatabefore processing is appropriate.src/app/modeler/data-mode/data-detail/data-detail.component.html (2)
160-240: LGTM! Comprehensive type-specific initial value editors.The type-specific editors appropriately handle each data type with suitable UI controls: toggle for boolean, number input for numbers, date/datetime pickers, select for enumerations/multichoice, and a fallback text input. The delegation to
nab-task-ref-init-fieldforTASK_REFmaintains good component separation.
119-124: No changes needed—the code correctly handles both event types.The code at line 120 already uses an
instanceofcheck to distinguish betweenMatAutocompleteSelectedEventand regularEvent, accessing the appropriate property for each type:$event.option.valuefor autocomplete selection and$event.target.valuefor change events. Both event types are handled correctly.src/app/dialogs/dialog-marking-change/dialog-marking-change.component.ts (1)
14-28: LGTM! Well-structured dialog component.The component properly injects dialog data and the simulation service, initializes the place reference and form control with appropriate validators. The pattern aligns with similar dialog components in the codebase.
src/app/modeler/simulation-mode/tool/simulation-tool.ts (1)
58-64: LGTM - Dialog interaction handlers are well structured.The
setTimeout(..., 0)pattern for opening dialogs after pointer events is appropriate for avoiding Windows context menu issues as noted in the comment. The arc handler correctly differentiates between data references and place references.Also applies to: 72-91
src/app/modeler/services/model/model.service.ts (3)
451-484: Comprehensive action collection implementation.The
collectActionsmethod thoroughly traverses all action sources: case events, process events, role events, transition events (including nested dataRef events), and data set events. This ensures the sequence generator is properly initialized with existing action IDs.
525-542: Improved reference value resolution with proper type checking.The updated
getReferenceValuemethod correctly usesImportUtils.isInitValueNumberto validate numeric init values and accepts an optional model parameter for flexibility. Returning0for non-numeric values is a safe default.
447-449:nextActionId()lacks uniqueness verification by design, not oversight.Unlike other
next*Id()methods, this approach is intentional because actions are distributed across events throughout the model rather than stored in a centralized collection. There is nogetAction(id)method to enable efficient uniqueness checks likegetPlace(),getTransition(), etc. The_actionIdSequenceis instead pre-populated during reset with all existing actions collected from case events, process events, role events, transition events, data reference events, and the dataset, preventing collisions during normal operation.Likely an incorrect or invalid review comment.
src/app/form-builder/field-list/field-list.service.ts (2)
6-25: Well-defined interfaces for field metadata.The new
PropertyDef,ComponentDef, andDataRefDefinterfaces provide clear type definitions for the field configuration system, improving type safety across the form builder.
409-424: Type assertion forstringCollectionis valid.
stringCollectionis a recognized value in the Petriflow DataType specification and is a legitimate use case. The type assertion is appropriate and does not bypass type checking for invalid data.Likely an incorrect or invalid review comment.
src/app/form-builder/edit-panel/edit-panel.component.html (2)
154-156: Good refactor using dedicated component for TaskRef initialization.Replacing the inline TaskRef autocomplete block with the
nab-task-ref-init-fieldcomponent improves maintainability and reusability.
202-220: Autocomplete integration for component/property selection looks good.The autocomplete functionality for component names and properties provides a better UX with filtered suggestions. The "Copy to DataRef" action button enables convenient property transfer between dataVariable and dataRef.
Also applies to: 242-246
src/app/form-builder/edit-panel/edit-panel.component.ts (4)
38-39: LGTM on imports.The new imports for
FieldListServicetypes andMatAutocompleteSelectedEventare correctly added to support the autocomplete-driven UI.
418-425: Good enhancement to support both autocomplete and direct input.The dual handling of
MatAutocompleteSelectedEventand direct input events provides flexibility for both autocomplete selection and manual entry.
455-475: LGTM on setComponent and setDataRefComponent enhancements.Both methods correctly handle
MatAutocompleteSelectedEventand direct input, with appropriate null checks for creating the component if it doesn't exist.
569-574: Remove unusedclonePropertymethod or implement error handling for missingclone()methods.The
clonePropertymethod is not called anywhere in the codebase and is therefore dead code. Additionally, without access to the @netgrif/petriflow library source, the assumption that all properties onDataRefandDataVariablehave aclone()method cannot be verified. Either remove this method if no longer needed, or add type guards and error handling to safely handle properties that may lack aclone()method.src/app/modeler/data-mode/data-detail/data-detail.component.ts (3)
199-207: LGTM on dynamic-init handling.The logic correctly initializes
item.initwith an emptyI18nWithDynamicwhen undefined, and properly creates theinitsarray when dynamic mode is enabled.
254-277: LGTM on new value setter methods.The
setBooleanValue,setNumberValue,formatDate, andformatDateTimemethods provide clean, type-specific handling for different field types.
238-243: Good dual-handling pattern for autocomplete and direct input.The
property_keycase correctly distinguishes betweenMatAutocompleteSelectedEventand direct input events, consistent with the pattern used inedit-panel.component.ts.
| "netgrif": "https://netgrif.com", | ||
| "bpmn2pn": "https://bpmn2pn.netgrif.cloud/bpmn2pn/", | ||
| "engine": "https://demo.netgrif.com/", | ||
| "engine": "https://demo.netgrif.cloud/", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine nae.json file
find . -name "nae.json" -type fRepository: netgrif/application-builder
Length of output: 81
🏁 Script executed:
# Examine nae.json around line 108
head -120 ./nae.json | tail -30Repository: netgrif/application-builder
Length of output: 947
🏁 Script executed:
#!/bin/bash
# Check if the new engine URL is accessible
echo "Testing new engine URL..."
HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" "https://demo.netgrif.cloud/" --max-time 10)
if [ "$HTTP_CODE" -ge 200 ] && [ "$HTTP_CODE" -lt 400 ]; then
echo "✓ URL is accessible (HTTP $HTTP_CODE)"
else
echo "✗ URL returned HTTP $HTTP_CODE or is unreachable"
fi
echo ""
echo "Comparing with old URL..."
OLD_HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" "https://demo.netgrif.com/" --max-time 10)
echo "Old URL status: HTTP $OLD_HTTP_CODE"Repository: netgrif/application-builder
Length of output: 190
Do not merge: New engine URL is unreachable.
The engine URL has been changed from demo.netgrif.com to demo.netgrif.cloud, but the new endpoint is not accessible (HTTP 000). The old URL remains live and functional (HTTP 200). This change will break production deployments. Either revert to the original URL or ensure the new demo.netgrif.cloud endpoint is properly deployed and accessible before merging.
🤖 Prompt for AI Agents
In nae.json at line 108, the engine URL was changed to
"https://demo.netgrif.cloud/" but that endpoint is currently unreachable; revert
the value back to the original reachable URL "https://demo.netgrif.com/" OR
verify and deploy the demo.netgrif.cloud service and only then update this file.
After reverting or fixing the deployment, run a connectivity/HTTP health check
from CI or locally to confirm a 200 response before committing the change.
package.json
Outdated
| { | ||
| "name": "@netgrif/application-builder", | ||
| "version": "2.0.0", | ||
| "version": "2.1.0-RC.7", |
There was a problem hiding this comment.
Version mismatch: RC version in release PR.
The PR is titled "Release 2.1.0" targeting the master branch, but the package version is set to "2.1.0-RC.7" (release candidate). Release candidate versions should typically not be merged to master. Ensure the version is updated to "2.1.0" (without the RC suffix) before merging this release.
Apply this diff to use the final release version:
- "version": "2.1.0-RC.7",
+ "version": "2.1.0",📝 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.
| "version": "2.1.0-RC.7", | |
| "version": "2.1.0", |
🤖 Prompt for AI Agents
In package.json around line 3, the version field is set to the RC value
"2.1.0-RC.7" but this PR is a final release; update the version string to
"2.1.0" (remove the "-RC.7" suffix) so the package.json reflects the final
release before merging to master, and ensure no other files or lockfiles
reference the RC tag.
| <mat-form-field fxFlex *ngFor="let data of dataSet"> | ||
| <mat-label>{{data.id}}</mat-label> | ||
| <input type="number" step="1" matInput [(ngModel)]="data.value"> | ||
| <input type="number" step="1" min="0" matInput [(ngModel)]="data.value" [formControl]="valueCtrl"> | ||
| <mat-error *ngIf="valueCtrl.errors && valueCtrl.errors.required">Value is required</mat-error> | ||
| <mat-error *ngIf="valueCtrl.errors && valueCtrl.errors.validValue">Value can not be a negative number</mat-error> | ||
| </mat-form-field> |
There was a problem hiding this comment.
Critical: Shared FormControl breaks multi-field editing.
All input fields in the loop share the same valueCtrl FormControl. This means:
- All inputs display and update the same value
- Validation state is shared across all fields
- Users cannot edit multiple data values independently
Each data item needs its own FormControl or the component should use template-driven forms with separate ngModel bindings without a shared FormControl.
Since the AI summary indicates valueCtrl was introduced in the TypeScript file, you'll need to refactor the component to either:
Option 1: Remove the FormControl and use template-driven validation:
- <input type="number" step="1" min="0" matInput [(ngModel)]="data.value" [formControl]="valueCtrl">
- <mat-error *ngIf="valueCtrl.errors && valueCtrl.errors.required">Value is required</mat-error>
- <mat-error *ngIf="valueCtrl.errors && valueCtrl.errors.validValue">Value can not be a negative number</mat-error>
+ <input type="number" step="1" min="0" matInput [(ngModel)]="data.value" required>
+ <mat-error>Value is required and cannot be negative</mat-error>And update the save button to check if any data.value is invalid.
Option 2: Use FormArray with individual controls for each data item (requires more changes in the TypeScript file).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app/dialogs/dialog-change-data/dialog-change-data.component.html around
lines 3-8, the template currently binds every input to the same FormControl
(valueCtrl) causing shared value and validation across all rows; fix by giving
each datum its own control: either remove the shared FormControl and rely on
template-driven ngModel-only inputs (remove [formControl]="valueCtrl", keep
[(ngModel)]="data.value", add required and min attributes and update the save
routine to validate that no data.value is null/negative), or implement a
reactive FormArray in the component TS that creates one FormControl per data
item and bind each input to formControlName/index accordingly (populate the
FormArray from dataSet, validate per-control, and use the FormArray.valid flag
on save).
| import {CanvasTransition} from '../../modeler/edit-mode/domain/canvas-transition'; | ||
| import {CanvasNodeElement} from '../../modeler/edit-mode/domain/canvas-node-element'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if CanvasTransition and CanvasNodeElement are used in the component
# First, let's see the full component file to check usage
echo "=== Checking file: src/app/dialogs/dialog-change-data/dialog-change-data.component.ts ==="
wc -l src/app/dialogs/dialog-change-data/dialog-change-data.component.ts
echo -e "\n=== Full file content ==="
cat -n src/app/dialogs/dialog-change-data/dialog-change-data.component.ts
echo -e "\n=== Searching for CanvasTransition or CanvasNodeElement usage ==="
rg -n 'CanvasTransition|CanvasNodeElement' src/app/dialogs/dialog-change-data/dialog-change-data.component.tsRepository: netgrif/application-builder
Length of output: 2264
Remove unused imports on lines 4-5.
CanvasTransition and CanvasNodeElement are imported but not used in the component. Delete both import statements.
🤖 Prompt for AI Agents
In src/app/dialogs/dialog-change-data/dialog-change-data.component.ts around
lines 4 to 5, the imports for CanvasTransition and CanvasNodeElement are unused;
remove those two import statements to clean up the file and ensure no unused
imports remain (also run/verify TypeScript linting to confirm no other unused
imports).
| private validValue(): ValidatorFn { | ||
| return (fc: FormControl): { [key: string]: any } | null => { | ||
| const value = Math.floor(fc.value as number); | ||
| if (value !== Infinity && value === fc.value as number && value >= 0) { | ||
| return null; | ||
| } | ||
| return ({validMultiplicity: true}) | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider clarifying validator logic and edge cases.
The validator correctly enforces non-negative integers, but consider these edge cases:
- -Infinity: Line 40 checks
value !== Infinitybut not!isFinite(value), allowing-Infinityto potentially pass if it somehow equals itself afterMath.floor. - Validator naming: The error key is
validMultiplicity, which suggests this is for multiplicity values. If multiplicity must be strictly positive (>= 1), adjust the condition tovalue >= 1.
Apply this diff for more robust validation:
private validValue(): ValidatorFn {
return (fc: FormControl): { [key: string]: any } | null => {
const value = Math.floor(fc.value as number);
- if (value !== Infinity && value === fc.value as number && value >= 0) {
+ if (isFinite(value) && value === fc.value as number && value >= 0) {
return null;
}
return ({validMultiplicity: true})
};
}🤖 Prompt for AI Agents
In src/app/dialogs/dialog-change-data/dialog-change-data.component.ts around
lines 37 to 45, the validator misses non-finite values (e.g., -Infinity) and may
have the wrong threshold for multiplicity; coerce the control value to a Number,
use Number.isFinite to reject infinities/NaN before flooring, compare the
floored number to the numeric original, and enforce the correct lower bound (use
value >= 1 if multiplicity must be strictly positive, otherwise keep value >=
0); if you change the bound, update or rename the error key (validMultiplicity)
to reflect the stricter requirement.
| reset() { | ||
| super.reset(); | ||
| this.restart(); | ||
| this.selectedElements.clear(); | ||
| this.clipboardElements.clear(); | ||
| this.deselectAll(); | ||
| } |
There was a problem hiding this comment.
Critical ordering bug: Elements will remain visually active after reset.
Calling this.selectedElements.clear() on line 94 before this.deselectAll() on line 96 prevents proper visual deactivation. Here's why:
deselectAll()callsclearSelection(), which iterates overselectedElements.getAll()to callremoveFromSelection()on each elementremoveFromSelection()both removes the element from the collection AND callsdeactivate()to reset visual styles (cursor, highlighting)- If
selectedElements.clear()is called first, the collection is empty whendeselectAll()runs - Result: Elements remain visually activated (highlighted, with 'move' cursor) even though they're no longer selected internally
This creates an inconsistent state where users see highlighted elements they cannot interact with as selected.
Apply this diff to fix the ordering:
reset() {
super.reset();
this.restart();
- this.selectedElements.clear();
this.clipboardElements.clear();
this.deselectAll();
}Note: The explicit selectedElements.clear() call is redundant because deselectAll() → clearSelection() → removeFromSelection() already clears the collection while properly deactivating elements.
📝 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.
| reset() { | |
| super.reset(); | |
| this.restart(); | |
| this.selectedElements.clear(); | |
| this.clipboardElements.clear(); | |
| this.deselectAll(); | |
| } | |
| reset() { | |
| super.reset(); | |
| this.restart(); | |
| this.clipboardElements.clear(); | |
| this.deselectAll(); | |
| } |
🤖 Prompt for AI Agents
In src/app/modeler/edit-mode/services/modes/select-tool.ts around lines 91 to
97, the reset() method clears selectedElements before calling deselectAll(),
which prevents removeFromSelection() from deactivating elements and leaves them
visually active; fix by removing the explicit this.selectedElements.clear() and
calling this.deselectAll() immediately after this.restart() (so selection
removal/deactivation runs before you clear clipboardElements), e.g. keep
super.reset(); this.restart(); this.deselectAll();
this.clipboardElements.clear();.
| <input matInput [(ngModel)]="role.role.id" (change)="changeId($event)" [formControl]="roleIdForm"> | ||
| <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.required">Id is required</mat-error> | ||
| <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.validUnique"> | ||
| Transition with given id already exists | ||
| </mat-error> |
There was a problem hiding this comment.
Fix incorrect entity type in error message.
The error message on line 10 references "Transition" but should reference "Role" since this is the role detail component.
Apply this diff to correct the error message:
- <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.validUnique">
- Transition with given id already exists
- </mat-error>
+ <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.validUnique">
+ Role with given id already exists
+ </mat-error>📝 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.
| <input matInput [(ngModel)]="role.role.id" (change)="changeId($event)" [formControl]="roleIdForm"> | |
| <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.required">Id is required</mat-error> | |
| <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.validUnique"> | |
| Transition with given id already exists | |
| </mat-error> | |
| <input matInput [(ngModel)]="role.role.id" (change)="changeId($event)" [formControl]="roleIdForm"> | |
| <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.required">Id is required</mat-error> | |
| <mat-error *ngIf="roleIdForm.errors && roleIdForm.errors.validUnique"> | |
| Role with given id already exists | |
| </mat-error> |
🤖 Prompt for AI Agents
In src/app/modeler/role-mode/role-detail/role-detail.component.html around lines
7 to 11, the mat-error message incorrectly refers to "Transition" instead of
"Role"; update the text inside the mat-error (line 10) to read "Role with given
id already exists" so the error message correctly references the Role entity.
| changeGlobalFlag($event): void { | ||
| this.role.role.global = $event.checked; | ||
| this.shouldSave = true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add type annotation to the event parameter.
The changeGlobalFlag method follows the correct pattern, but the $event parameter should be explicitly typed for better type safety and IDE support.
Apply this diff to add the type annotation:
- changeGlobalFlag($event): void {
+ changeGlobalFlag($event: MatCheckboxChange): void {
this.role.role.global = $event.checked;
this.shouldSave = true;
}Also add the import at the top of the file:
import {MatCheckboxChange} from '@angular/material/checkbox';Note: The same type annotation improvement could be applied to changeId and changeTitle methods (they should receive typed event objects), but that's outside the scope of this change.
🤖 Prompt for AI Agents
In src/app/modeler/role-mode/role-detail/role-detail.component.ts around lines
78 to 81, the changeGlobalFlag method's $event parameter lacks a type
annotation; add an explicit type MatCheckboxChange to the $event parameter and
mark the method signature accordingly, and add the import "MatCheckboxChange"
from "@angular/material/checkbox" at the top of the file so the type resolves;
keep the method body unchanged and ensure TypeScript compiles with the new
import and typed parameter.
| openDataDialog(id: string, value: number): void { | ||
| const dataSet = new Map<string, number>([[id, value]]); | ||
| this.openDialog(DialogChangeDataComponent, { | ||
| width: '50%', | ||
| panelClass: "dialog-width-50", | ||
| data: { | ||
| dataSet: dataSet | ||
| } as DataSet | ||
| }, (data: Array<Data>) => { | ||
| if (data) { | ||
| this.simulationModeService.data.set(data[0].id, data[0].value); | ||
| this.simulation.updateData(this.simulationModeService.data); | ||
| this.simulationModeService.renderModel(this.simulation.simulationModel); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing bounds check before accessing data[0].
The callback checks if (data) but doesn't verify the array has elements before accessing data[0]. If the dialog returns an empty array, this will cause a runtime error.
openDataDialog(id: string, value: number): void {
const dataSet = new Map<string, number>([[id, value]]);
this.openDialog(DialogChangeDataComponent, {
width: '50%',
panelClass: "dialog-width-50",
data: {
dataSet: dataSet
} as DataSet
}, (data: Array<Data>) => {
- if (data) {
+ if (data && data.length > 0) {
this.simulationModeService.data.set(data[0].id, data[0].value);
this.simulation.updateData(this.simulationModeService.data);
this.simulationModeService.renderModel(this.simulation.simulationModel);
}
});
}🤖 Prompt for AI Agents
In src/app/modeler/simulation-mode/tool/simulation-tool.ts around lines 109 to
124, the callback checks only that `data` is truthy but then accesses `data[0]`
without ensuring the array has elements; add a bounds check (e.g., `if (data &&
data.length > 0)`) before accessing `data[0]`, and optionally validate
`data[0].id` and `data[0].value` are defined before using them to update
simulation state and render the model.
| <nab-root></nab-root> | ||
| <nab-root> | ||
|
|
||
| <!-- loading layout replaced by app after startupp --> |
There was a problem hiding this comment.
Fix the typo in the comment.
The comment contains a typo: "startupp" should be "startup".
Apply this diff to fix the typo:
- <!-- loading layout replaced by app after startupp -->
+ <!-- loading layout replaced by app after startup -->📝 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.
| <!-- loading layout replaced by app after startupp --> | |
| <!-- loading layout replaced by app after startup --> |
🤖 Prompt for AI Agents
In src/index.html around line 74, the HTML comment contains a typo ("startupp");
update the comment text to read "startup" so it becomes: <!-- loading layout
replaced by app after startup -->.
| filteredComponents(component: PetriflowComponent): Array<ComponentDef> { | ||
| const componentDefs: DataRefDef = this._fieldListService.fieldListArray.find(type => type.type === this.dataVariable.type); | ||
| if (!componentDefs) { | ||
| return []; | ||
| } | ||
| return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(component.name)); | ||
| } |
There was a problem hiding this comment.
Potential null reference when component.name is undefined.
The filter at line 544 calls component.name directly without a null check. If component.name is undefined, includes(undefined) will coerce it to the string "undefined" and may produce unexpected matches.
filteredComponents(component: PetriflowComponent): Array<ComponentDef> {
const componentDefs: DataRefDef = this._fieldListService.fieldListArray.find(type => type.type === this.dataVariable.type);
if (!componentDefs) {
return [];
}
- return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(component.name));
+ return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(component.name?.toLowerCase() ?? ''));
}This also normalizes the case for consistent filtering.
📝 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.
| filteredComponents(component: PetriflowComponent): Array<ComponentDef> { | |
| const componentDefs: DataRefDef = this._fieldListService.fieldListArray.find(type => type.type === this.dataVariable.type); | |
| if (!componentDefs) { | |
| return []; | |
| } | |
| return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(component.name)); | |
| } | |
| filteredComponents(component: PetriflowComponent): Array<ComponentDef> { | |
| const componentDefs: DataRefDef = this._fieldListService.fieldListArray.find(type => type.type === this.dataVariable.type); | |
| if (!componentDefs) { | |
| return []; | |
| } | |
| return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(component.name?.toLowerCase() ?? '')); | |
| } |
🤖 Prompt for AI Agents
In src/app/form-builder/edit-panel/edit-panel.component.ts around lines 539 to
545, the filter uses component.name directly which can be undefined and causes
includes(undefined) to behave incorrectly; update the filter to normalize both
sides and guard against undefined by deriving a safeLowerName = (component.name
|| '').toLowerCase() and only calling includes on that, and ensure def.title is
present (e.g., (def.title || '').toLowerCase().includes(safeLowerName)) so
filtering is consistent and null-safe.
| public setPropertyDefaultValue($event: MatAutocompleteSelectedEvent, index: number, component: PetriflowComponent): void { | ||
| component.properties[index].value = this._fieldListService.fieldListArray | ||
| .find(type => type.type === this.dataVariable.type)?.components | ||
| .find(compDef => (!component.name && !compDef.name) || (!!component.name && !!compDef.name && component.name === compDef.name))?.properties | ||
| .find(propDef => propDef.name === $event.option.value).defaultValue; | ||
| } |
There was a problem hiding this comment.
Missing null check before accessing defaultValue can cause runtime error.
The chained find() calls may return undefined, but .defaultValue is accessed unconditionally on line 566. If no matching property definition is found, this will throw a TypeError.
public setPropertyDefaultValue($event: MatAutocompleteSelectedEvent, index: number, component: PetriflowComponent): void {
- component.properties[index].value = this._fieldListService.fieldListArray
+ const defaultValue = this._fieldListService.fieldListArray
.find(type => type.type === this.dataVariable.type)?.components
.find(compDef => (!component.name && !compDef.name) || (!!component.name && !!compDef.name && component.name === compDef.name))?.properties
- .find(propDef => propDef.name === $event.option.value).defaultValue;
+ ?.find(propDef => propDef.name === $event.option.value)?.defaultValue;
+ if (defaultValue !== undefined) {
+ component.properties[index].value = defaultValue;
+ }
}🤖 Prompt for AI Agents
In src/app/form-builder/edit-panel/edit-panel.component.ts around lines 562 to
567, the chained find() calls may return undefined and the code accesses
.defaultValue unconditionally which can throw; update the logic to safely handle
missing results by resolving each find into a temporary variable (or use
optional chaining) and only read .defaultValue if the property definition
exists, otherwise assign a sensible fallback (e.g., undefined or empty string)
to component.properties[index].value; ensure you don’t assume intermediate
objects exist before accessing their properties.
| import { | ||
| ComponentDef, | ||
| DataRefDef, | ||
| FieldListService, | ||
| PropertyDef | ||
| } from '../../../form-builder/field-list/field-list.service'; | ||
| import {MatAutocompleteSelectedEvent} from '@angular/material/autocomplete'; | ||
| import {MatChipInputEvent} from '@angular/material/chips'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MatChipInputEvent usage in the component file and template
rg -n "MatChipInputEvent" src/app/modeler/data-mode/data-detail/ --type tsRepository: netgrif/application-builder
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Search more broadly for any use of MatChipInputEvent (method params, variable declarations, etc.)
# Also check if there's a template file that might use it
rg "MatChipInputEvent" src/app/modeler/data-mode/data-detail/ -A 2 -B 2
echo "---"
echo "Checking for template file:"
find src/app/modeler/data-mode/data-detail/ -name "*.html"Repository: netgrif/application-builder
Length of output: 710
🏁 Script executed:
#!/bin/bash
# Check the template file for any chip-related content
cat src/app/modeler/data-mode/data-detail/data-detail.component.html | grep -i "chip\|matChip" || echo "No chip-related content found in template"Repository: netgrif/application-builder
Length of output: 112
Remove unused import: MatChipInputEvent.
MatChipInputEvent is imported on line 35 but is not used anywhere in the component (no method parameters, variable declarations) or template. Remove this import.
🤖 Prompt for AI Agents
In src/app/modeler/data-mode/data-detail/data-detail.component.ts around lines
28 to 35, the import MatChipInputEvent is unused; remove MatChipInputEvent from
the import list (delete it from the line importing from
'@angular/material/chips') so the file only imports symbols that are actually
used, then run a quick build/TS lint to confirm no unused-import errors remain.
| get filteredComponents(): Array<ComponentDef> { | ||
| const componentDefs: DataRefDef = this._fieldListService.fieldListArray.find(type => type.type === this.item.type); | ||
| if (!componentDefs) { | ||
| return []; | ||
| } | ||
| return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(this.item.component.name)); | ||
| } |
There was a problem hiding this comment.
Potential null reference and case-sensitivity issue in filteredComponents.
Similar to edit-panel.component.ts, line 418 uses this.item.component.name directly in includes() without null-checking or case normalization. If component.name is undefined, it will be coerced to "undefined".
get filteredComponents(): Array<ComponentDef> {
const componentDefs: DataRefDef = this._fieldListService.fieldListArray.find(type => type.type === this.item.type);
if (!componentDefs) {
return [];
}
- return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(this.item.component.name));
+ return componentDefs.components.filter(def => def.name !== undefined && def.title.toLowerCase().includes(this.item.component?.name?.toLowerCase() ?? ''));
}🤖 Prompt for AI Agents
In src/app/modeler/data-mode/data-detail/data-detail.component.ts around lines
413 to 419, the getter uses this.item.component.name directly in includes()
which can be undefined and is not case-normalized; update the code to (1) guard
against a missing component/name by using safe access or a default empty string
(e.g., const query = (this.item?.component?.name || '').toLowerCase()), and (2)
compare using lowercased strings (e.g.,
def.title?.toLowerCase().includes(query)), ensuring you also null-check
def.title before calling toLowerCase so no null reference errors occur.
| public setPropertyDefaultValue($event: MatAutocompleteSelectedEvent, index: number, dataVariable: DataVariable): void { | ||
| dataVariable.component.properties[index].value = this._fieldListService.fieldListArray | ||
| .find(type => type.type === dataVariable.type)?.components | ||
| .find(compDef => (!dataVariable.component.name && !compDef.name) || (!!dataVariable.component.name && !!compDef.name && dataVariable.component.name === compDef.name))?.properties | ||
| .find(propDef => propDef.name === $event.option.value).defaultValue; | ||
| } |
There was a problem hiding this comment.
Missing null check before accessing defaultValue can cause runtime error.
Same issue as in edit-panel.component.ts: the chained find() on line 440 may return undefined, causing a TypeError when accessing .defaultValue.
public setPropertyDefaultValue($event: MatAutocompleteSelectedEvent, index: number, dataVariable: DataVariable): void {
- dataVariable.component.properties[index].value = this._fieldListService.fieldListArray
+ const defaultValue = this._fieldListService.fieldListArray
.find(type => type.type === dataVariable.type)?.components
.find(compDef => (!dataVariable.component.name && !compDef.name) || (!!dataVariable.component.name && !!compDef.name && dataVariable.component.name === compDef.name))?.properties
- .find(propDef => propDef.name === $event.option.value).defaultValue;
+ ?.find(propDef => propDef.name === $event.option.value)?.defaultValue;
+ if (defaultValue !== undefined) {
+ dataVariable.component.properties[index].value = defaultValue;
+ }
}📝 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.
| public setPropertyDefaultValue($event: MatAutocompleteSelectedEvent, index: number, dataVariable: DataVariable): void { | |
| dataVariable.component.properties[index].value = this._fieldListService.fieldListArray | |
| .find(type => type.type === dataVariable.type)?.components | |
| .find(compDef => (!dataVariable.component.name && !compDef.name) || (!!dataVariable.component.name && !!compDef.name && dataVariable.component.name === compDef.name))?.properties | |
| .find(propDef => propDef.name === $event.option.value).defaultValue; | |
| } | |
| public setPropertyDefaultValue($event: MatAutocompleteSelectedEvent, index: number, dataVariable: DataVariable): void { | |
| const defaultValue = this._fieldListService.fieldListArray | |
| .find(type => type.type === dataVariable.type)?.components | |
| .find(compDef => (!dataVariable.component.name && !compDef.name) || (!!dataVariable.component.name && !!compDef.name && dataVariable.component.name === compDef.name))?.properties | |
| ?.find(propDef => propDef.name === $event.option.value)?.defaultValue; | |
| if (defaultValue !== undefined) { | |
| dataVariable.component.properties[index].value = defaultValue; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/app/modeler/data-mode/data-detail/data-detail.component.ts around lines
436 to 441, the chained find() calls may return undefined and the code accesses
.defaultValue without checking, causing a runtime TypeError; fix by resolving
each step into guarded intermediate variables (typeDef, compDef, propDef) or use
safe optional checks so you only read .defaultValue when propDef is non-null,
and assign a sensible fallback (e.g., null or empty string) if any lookup fails.
- fix according to PR
[NAB-389] Cannot change the number of columns in form editor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/app/form-builder/gridster/gridster.service.ts (3)
244-244:JSON.parsemay throw on empty or invalid string.
event.dataTransfer.getData('properties')can return an empty string, andJSON.parse('')throws aSyntaxError. This was previously flagged but remains unaddressed.Wrap the parse in a try-catch or validate the string first:
- const properties: Array<PropertyDef> = JSON.parse(event.dataTransfer.getData('properties')); + let properties: Array<PropertyDef> = null; + try { + const propertiesData = event.dataTransfer.getData('properties'); + if (propertiesData) { + properties = JSON.parse(propertiesData); + } + } catch (e) { + // Invalid properties data, skip + }
244-249: Potential null reference when accessingnewDataRef.component.properties.If
ref_componentfrom drag data is empty/falsy,addDataRef(lines 220-222) won't create aComponent, leavingnewDataRef.componentasundefined. However, line 247 unconditionally accessesnewDataRef.component.properties, which will throw aTypeError. This was previously flagged but remains unaddressed.Add a null check or ensure component exists before pushing properties:
const properties: Array<PropertyDef> = JSON.parse(event.dataTransfer.getData('properties')); - if (!!properties) { + if (properties && newDataRef.component) { for (const property of properties) { newDataRef.component.properties.push(new Property(property.name, property.defaultValue)); } }Alternatively, ensure a component is always created in
addDataRefwhen properties will be added, or initialize one inaddNewDataRefif missing:if (!newDataRef.component) { newDataRef.component = new Component(''); newDataRef.component.properties = []; }
245-245: Redundant double negation!!properties.The
!!is unnecessary here;if (properties)achieves the same result.- if (!!properties) { + if (properties) {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/form-builder/form-builder.component.ts(2 hunks)src/app/form-builder/gridster/gridster.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/form-builder/form-builder.component.ts (2)
src/app/modeler/services/canvas/canvas-listener-tool.ts (2)
router(424-426)transitionService(428-430)src/app/modeler/modeler-config.ts (1)
ModelerConfig(3-40)
src/app/form-builder/gridster/gridster.service.ts (1)
src/app/form-builder/field-list/field-list.service.ts (1)
PropertyDef(6-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (4)
src/app/form-builder/gridster/gridster.service.ts (2)
21-21: LGTM: Import additions support property handling.The new imports for
PropertyandPropertyDefare necessary for the enhanced property-handling logic inaddNewDataRef.Also applies to: 29-29
199-199: LGTM: Return type change improves composability.Changing
addDataRefto returnDataRefallows the caller to capture and further process the created reference, which is leveraged inaddNewDataRef.Also applies to: 234-234
src/app/form-builder/form-builder.component.ts (2)
5-7: LGTM! Import statements are correctly added.The new imports support the default data group initialization logic and are consistent with the codebase patterns shown in the relevant code snippets.
18-18: Constructor signature updated correctly.The injection of
SelectedTransitionServicefollows standard Angular patterns. However, note that the constructor now contains initialization logic that mutates the model (see next comment).
| if (this.modelService.model.getTransition(this.transitionService.id)?.dataGroups?.length === 0) { | ||
| const dataGroup = new DataGroup(`${this.transitionService.id}_0`); | ||
| dataGroup.layout = LayoutType.GRID; | ||
| dataGroup.cols = ModelerConfig.LAYOUT_DEFAULT_COLS; | ||
| this.modelService.model.getTransition(this.transitionService.id).dataGroups.push(dataGroup); | ||
| } |
There was a problem hiding this comment.
Critical: Null-pointer risk and constructor side effects.
This code has several issues:
-
Null-pointer exception risk (Line 26): The second call to
getTransitiondoesn't use optional chaining (?.), which will throw if it returnsnullorundefined. While Line 22 uses?., Line 26 directly accesses.dataGroups.push(...)without null safety. -
Inconsistent null handling: The condition
?.length === 0only triggers for an empty array, not fornullorundefineddataGroups. If dataGroups isnull/undefined, the condition evaluates tofalse, but then Line 26 would still fail when trying to push. -
Duplicate calls:
this.modelService.model.getTransition(this.transitionService.id)is called twice—once with optional chaining and once without. This is inefficient and error-prone. -
Constructor side effects: Constructors should be lightweight and avoid model mutations. This initialization logic should be moved to
ngOnInitlifecycle hook.
Apply this diff to fix the null safety issues, eliminate duplicate calls, and move logic to ngOnInit:
-export class FormBuilderComponent implements AfterViewInit {
+export class FormBuilderComponent implements OnInit, AfterViewInit {
title = 'form-builder';
width: number;
constructor(private router: Router, private modelService: ModelService, private transitionService: SelectedTransitionService) {
if (!this.modelService.model) {
this.router.navigate(['/modeler']);
}
- if (this.modelService.model.getTransition(this.transitionService.id)?.dataGroups?.length === 0) {
- const dataGroup = new DataGroup(`${this.transitionService.id}_0`);
- dataGroup.layout = LayoutType.GRID;
- dataGroup.cols = ModelerConfig.LAYOUT_DEFAULT_COLS;
- this.modelService.model.getTransition(this.transitionService.id).dataGroups.push(dataGroup);
- }
}
+ ngOnInit(): void {
+ const transition = this.modelService.model?.getTransition(this.transitionService.id);
+ if (transition && (!transition.dataGroups || transition.dataGroups.length === 0)) {
+ const dataGroup = new DataGroup(`${this.transitionService.id}_0`);
+ dataGroup.layout = LayoutType.GRID;
+ dataGroup.cols = ModelerConfig.LAYOUT_DEFAULT_COLS;
+ if (!transition.dataGroups) {
+ transition.dataGroups = [];
+ }
+ transition.dataGroups.push(dataGroup);
+ }
+ }
+
ngAfterViewInit(): void {Additionally, update the class declaration on Line 1 to include OnInit:
-import {AfterViewInit, Component, OnDestroy, OnInit} from '@angular/core';
+import {AfterViewInit, Component, OnInit} from '@angular/core';📝 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.
| if (this.modelService.model.getTransition(this.transitionService.id)?.dataGroups?.length === 0) { | |
| const dataGroup = new DataGroup(`${this.transitionService.id}_0`); | |
| dataGroup.layout = LayoutType.GRID; | |
| dataGroup.cols = ModelerConfig.LAYOUT_DEFAULT_COLS; | |
| this.modelService.model.getTransition(this.transitionService.id).dataGroups.push(dataGroup); | |
| } | |
| export class FormBuilderComponent implements OnInit, AfterViewInit { | |
| title = 'form-builder'; | |
| width: number; | |
| constructor(private router: Router, private modelService: ModelService, private transitionService: SelectedTransitionService) { | |
| if (!this.modelService.model) { | |
| this.router.navigate(['/modeler']); | |
| } | |
| } | |
| ngOnInit(): void { | |
| const transition = this.modelService.model?.getTransition(this.transitionService.id); | |
| if (transition && (!transition.dataGroups || transition.dataGroups.length === 0)) { | |
| const dataGroup = new DataGroup(`${this.transitionService.id}_0`); | |
| dataGroup.layout = LayoutType.GRID; | |
| dataGroup.cols = ModelerConfig.LAYOUT_DEFAULT_COLS; | |
| if (!transition.dataGroups) { | |
| transition.dataGroups = []; | |
| } | |
| transition.dataGroups.push(dataGroup); | |
| } | |
| } | |
| ngAfterViewInit(): void { | |
| // ... existing code ... | |
| } | |
| } |
| newDataRef.component.properties.push(new Property(property.name, property.defaultValue)); | ||
| } | ||
| } | ||
| this.options.api.optionsChanged(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider optional chaining for defensive coding.
While this.options.api should be available when this method is called (since it's only invoked after gridster initialization), using optional chaining would be more defensive and consistent with line 113.
- this.options.api.optionsChanged();
+ this.options.api?.optionsChanged();📝 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.
| this.options.api.optionsChanged(); | |
| this.options.api?.optionsChanged(); |
🤖 Prompt for AI Agents
In src/app/form-builder/gridster/gridster.service.ts around line 250, the direct
call this.options.api.optionsChanged() is fragile; update it to use optional
chaining on the api reference (this.options.api?.optionsChanged()) to guard
against undefined API objects, matching the defensive pattern used at line 113
and avoiding runtime errors if the method is invoked before full initialization.
Description
Release 2.1.0 with bug fixes
Dependencies
Third party dependencies
Blocking Pull requests
<Depends on #(PR id)>/
How Has Been This Tested?
Test Configuration
<Please describe configuration for tests to run if applicable, like program parameters, host OS, VM configuration etc. You can use >
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.