Conversation
WalkthroughRebuilds the Events tab in the dataList settingsForm: constructs a base event component, appends multiple configurators (including a new code editor for On List Item Save), groups selection-related configurables in a conditional container, and finalizes with toJson to produce the hierarchical event settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Designer as Designer UI
participant Builder as SettingsForm Builder
participant Config as ConfigurableAction
rect rgba(100,180,120,0.08)
Designer->>Builder: open Events tab
Builder->>Builder: construct base event component
Builder->>Config: addOnListItemSave (codeEditor) [onListItemSave]
Builder->>Config: addOnListItemSaveSuccessAction [onListItemSaveSuccessAction]
Builder->>Config: addRowEvents (save/delete/click/hover)
Builder->>Builder: create selection container (visible if selectionMode != "none")
Builder->>Config: addOnListItemSelect & onSelectionChange inside container
Builder->>Builder: assemble configurators
Builder->>Builder: toJson() -> final events JSON
end
note right of Builder: Result is hierarchical\nand conditionally-visible settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
shesha-reactjs/src/designer-components/dataList/settingsForm.ts (3)
444-451: Inconsistent parentId on configurators in Events tab.Some configurators omit
parentId: eventsTabIdwhile others include it. Add for consistency and to avoid layout/ownership quirks in the builder..addConfigurableActionConfigurator({ id: nanoid(), propertyName: 'onListItemSaveSuccessAction', label: 'On List Item Save Success', + parentId: eventsTabId, hideLabel: true, description: 'Custom Action configuration executed when saving list items (validation, calculations, etc.)', }) .addConfigurableActionConfigurator({ id: nanoid(), propertyName: 'onRowDeleteSuccessAction', label: 'On List Item Delete Success', description: 'Custom business logic to be executed after successfull deletion of a list item.', + parentId: eventsTabId, hideLabel: true, }) .addConfigurableActionConfigurator({ id: nanoid(), propertyName: 'onListItemClick', label: 'On List Item Click', description: 'Action to execute when a list item is clicked', + parentId: eventsTabId, }) .addConfigurableActionConfigurator({ id: nanoid(), propertyName: 'onListItemHover', label: 'On List Item Hover', description: 'Action to execute when hovering over a list item', + parentId: eventsTabId, })Also applies to: 462-468, 469-475, 476-481
17-48: exposedVariables are stringified; align type with other editors.
ROW_SAVE_EXPOSED_VARIABLESbuilds strings viaJSON.stringify, while other places (e.g., Appearance > Group Style) pass objects. Normalize to object[] for consistency unless the runtime explicitly requires strings.- const ROW_SAVE_EXPOSED_VARIABLES = [ - { - id: nanoid(), - name: 'data', - description: 'Current row data', - type: 'object', - }, - { - id: nanoid(), - name: 'formData', - description: 'Form values', - type: 'object', - }, - { - id: nanoid(), - name: 'globalState', - description: 'The global state of the application', - type: 'object', - }, - { - id: nanoid(), - name: 'http', - description: 'axios instance used to make http requests', - type: 'object', - }, - { - id: nanoid(), - name: 'moment', - description: 'The moment.js object', - type: 'object', - }, - ].map((item) => JSON.stringify(item)); + const ROW_SAVE_EXPOSED_VARIABLES = [ + { name: 'data', description: 'Current row data', type: 'object' }, + { name: 'formData', description: 'Form values', type: 'object' }, + { name: 'globalState', description: 'The global state of the application', type: 'object' }, + { name: 'http', description: 'axios instance used to make http requests', type: 'object' }, + { name: 'moment', description: 'The moment.js object', type: 'object' }, + ];If the designer intentionally expects string entries, consider converting other usages to be consistent instead. Please confirm the expected shape.
424-435: onListItemSave lacks exposedVariables; add for parity and usability.Expose the same variables as
onRowSaveso authors have a consistent API surface..addSettingsInputRow({ id: nanoid(), inputs: [{ id: nanoid(), type: 'codeEditor', propertyName: 'onListItemSave', label: 'On List Item Save', jsSetting: false, tooltip: 'Custom business logic executed when saving list items (validation, calculations, etc.)', + exposedVariables: ROW_SAVE_EXPOSED_VARIABLES, }], hideLabel: true, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/dataList/settingsForm.ts(1 hunks)
⏰ 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-attempt
🔇 Additional comments (1)
shesha-reactjs/src/designer-components/dataList/settingsForm.ts (1)
437-443: No action required — migration already implemented.The review concern about breaking changes from the property rename has already been addressed. The dataListComponent.tsx migration (version 5→6) maps the old
actionConfigurationto the newdblClickActionConfiguration, and version 6 appliesmigrateNavigateAction()transformation. The PR's addition of the settings form entry at line 438 is simply exposing an already-migrated and stable property through the designer UI. No persisted configs will lose wiring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
shesha-reactjs/src/designer-components/dataList/settingsForm.ts (2)
424-435: Add exposedVariables to help developers understand available context.The
onListItemSavecode editor lacks exposed variables documentation, while the similaronRowSavehandler (lines 451-461) includes them. Developers would benefit from knowing what variables are available in this context.Consider adding exposedVariables similar to
onRowSave:.addSettingsInputRow({ id: nanoid(), inputs: [{ id: nanoid(), type: 'codeEditor', propertyName: 'onListItemSave', label: 'On List Item Save', jsSetting: false, tooltip: 'Custom business logic executed when saving list items (validation, calculations, etc.)', + exposedVariables: [ + `{ name: "data", description: "Current row data", type: "object" }`, + `{ name: "formData", description: "Form values", type: "object" }`, + `{ name: "globalState", description: "The global state of the application", type: "object" }`, + `{ name: "http", description: "axios instance used to make http requests", type: "object" }`, + `{ name: "moment", description: "The moment.js object", type: "object" }`, + ], }], hideLabel: true, })
444-450: Clarify the description to reflect post-save timing.The description mentions "validation, calculations" which typically occur before saving, but this handler executes after a successful save. Consider clarifying the timing and typical use cases for post-save actions.
Suggested revision:
.addConfigurableActionConfigurator({ id: nanoid(), propertyName: 'onListItemSaveSuccessAction', label: 'On List Item Save Success', hideLabel: true, - description: 'Custom Action configuration executed when saving list items (validation, calculations, etc.)', + description: 'Action to execute after successfully saving a list item (e.g., showing notifications, updating related data, navigation)', })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/dataList/settingsForm.ts(1 hunks)
⏰ 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-attempt
🔇 Additional comments (6)
shesha-reactjs/src/designer-components/dataList/settingsForm.ts (6)
436-443: LGTM! Good addition of descriptive text.The double-click action configurator now includes a helpful description, improving the user experience.
451-461: LGTM! Well-documented handler with proper context.The
onRowSavehandler includes helpful tooltip, description, exposed variables, and appropriate conditional visibility. The note about returning an object or Promise is particularly useful for developers.
462-468: LGTM! Typo has been corrected.The typo flagged in the previous review ("successfull" → "successful") has been fixed. The description now correctly reads "successful deletion."
469-474: LGTM! Clear event handler definition.The click handler is well-defined with a clear description.
475-480: LGTM! Clear event handler definition.The hover handler is well-defined with a clear description.
481-503: LGTM! Well-structured conditional grouping of selection events.The approach of building base components with the builder pattern, then appending a conditional container is valid. The conditional visibility logic correctly hides selection-related events when
selectionModeis "none", and the descriptions clearly distinguish betweenonListItemSelect(select only) andonSelectionChange(select and unselect).
2 participants
#3001
Summary by CodeRabbit