fix: Reduce cycle deps by removing wrong exports #39268
Conversation
WalkthroughThis pull request refactors multiple modules by updating import paths for constants and types—primarily Changes
Sequence Diagram(s)sequenceDiagram
participant D as DropdownWidget Component
participant P as Props
participant C as changeSelectedOption()
D->>P: Receives new props update
alt Props (defaultOptionValue/option) changed
D->>C: Invoke changeSelectedOption()
else No change detected
D-->>D: Maintain current state
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/client/src/utils/NavigationSelector/JsChildren.ts (1)
24-38: Consider distinguishing between function and variable types.Currently, both functions and variables use
ENTITY_TYPE.JSACTION, only differentiated by theisfunctionflag. Consider creating distinct entity types for better type safety and clarity.- type: ENTITY_TYPE.JSACTION, + type: isfunction ? ENTITY_TYPE.JS_FUNCTION : ENTITY_TYPE.JS_VARIABLE,Also applies to: 40-56
app/client/src/ce/entities/DataTree/dataTreeAction.ts (1)
15-17: Address the TODO comment about eslint disable.The comment indicates technical debt that should be addressed. Would you like me to help create an issue to track this?
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts (1)
144-150: Good addition of expandedByDefault property!The property addition with clear comments helps control the default expansion state of the data section. Consider adding this property to other sections for consistency.
app/client/src/widgets/MapWidget/widget/index.tsx (1)
433-461: Well-structured static methods for widget configuration!The new static methods provide clear configuration for:
- Default properties mapping
- Meta properties mapping
- Derived properties mapping
- Stylesheet configuration with theme variables
Consider adding JSDoc comments to document the purpose and return types of these methods.
+/** + * Returns the mapping of default properties for the widget + * @returns Record of property mappings + */ static getDefaultPropertiesMap(): Record<string, any> { return { markers: "defaultMarkers", center: "mapCenter", }; }app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (2)
772-774: Address TODO comments and eslint-disable directives.There are two TODO comments with eslint-disable directives that should be addressed:
- Line 772: Generic eslint-disable for explicit any
- Line 789: Generic eslint-disable for explicit any
Consider using proper TypeScript types instead of
any.- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - static getMetaPropertiesMap(): Record<string, any> { + static getMetaPropertiesMap(): Record<string, unknown> { - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (value: any) => isString(value) || isFinite(value), + (value: string | number) => isString(value) || isFinite(value),Also applies to: 789-791
360-371: Improve validation message clarity.The validation message for
defaultOptionValuecould be more explicit about the supported formats. Consider separating the examples for better readability.validation: { type: ValidationTypes.FUNCTION, params: { fn: defaultOptionValueValidation, expected: { type: "Array of values", - example: ` "option1, option2" | ['option1', 'option2'] | [{ "label": "label1", "value": "value1" }]`, + example: ` + Format 1: "option1, option2" + Format 2: ['option1', 'option2'] + Format 3: [{ "label": "label1", "value": "value1" }] + `, autocompleteDataType: AutocompleteDataType.ARRAY, }, }, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx(2 hunks)app/client/src/ce/components/editorComponents/Debugger/ErrorLogs/getLogIconForEntity.tsx(1 hunks)app/client/src/ce/entities/DataTree/dataTreeAction.ts(1 hunks)app/client/src/ce/entities/DataTree/dataTreeJSAction.ts(1 hunks)app/client/src/ce/entities/DataTree/isDynamicEntity.ts(1 hunks)app/client/src/ce/reducers/uiReducers/explorerReducer.ts(1 hunks)app/client/src/ce/utils/FilterInternalProperties/getEntityPeekData.ts(1 hunks)app/client/src/components/editorComponents/CodeEditor/EvaluatedValuePopup.tsx(1 hunks)app/client/src/components/editorComponents/CodeEditor/commandsHelper.ts(1 hunks)app/client/src/components/editorComponents/CodeEditor/generateQuickCommands.tsx(1 hunks)app/client/src/components/editorComponents/CodeEditor/index.tsx(1 hunks)app/client/src/components/editorComponents/GlobalSearch/utils.tsx(1 hunks)app/client/src/components/formControls/DynamicTextFieldControl.tsx(1 hunks)app/client/src/constants/PropertyControlConstants.tsx(1 hunks)app/client/src/entities/Action/actionProperties.test.ts(1 hunks)app/client/src/entities/Action/actionProperties.ts(1 hunks)app/client/src/entities/DataTree/dataTreeFactory.ts(0 hunks)app/client/src/entities/DataTree/dataTreeWidget.test.ts(1 hunks)app/client/src/entities/DataTree/dataTreeWidget.ts(2 hunks)app/client/src/entities/Widget/utils.test.ts(1 hunks)app/client/src/entities/Widget/utils.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCheckboxGroupWidget/config/propertyPaneConfig/contentConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMenuButtonWidget/config/propertyPaneConfig/contentConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSRadioGroupWidget/config/propertyPaneConfig/contentConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSwitchGroupWidget/config/propertyPaneConfig/contentConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSTableWidget/config/propertyPaneConfig/contentConfig.ts(1 hunks)app/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.tsx(2 hunks)app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx(1 hunks)app/client/src/pages/Editor/Explorer/JSActions/JSActionContextMenu.tsx(2 hunks)app/client/src/pages/Editor/Explorer/Widgets/WidgetContextMenu.tsx(1 hunks)app/client/src/plugins/Linting/utils/getLintingErrors.ts(1 hunks)app/client/src/selectors/dataTreeSelectors.ts(1 hunks)app/client/src/selectors/navigationSelectors.ts(2 hunks)app/client/src/utils/FilterInternalProperties/__tests__/index.test.ts(1 hunks)app/client/src/utils/NavigationSelector/JsChildren.ts(1 hunks)app/client/src/utils/NavigationSelector/WidgetChildren.ts(1 hunks)app/client/src/utils/WidgetLoadingStateUtils.test.ts(1 hunks)app/client/src/utils/autocomplete/AutocompleteSortRules.ts(1 hunks)app/client/src/utils/autocomplete/CodemirrorTernService.ts(2 hunks)app/client/src/utils/autocomplete/__tests__/AutocompleteSortRules.test.ts(1 hunks)app/client/src/utils/autocomplete/__tests__/TernServer.test.ts(1 hunks)app/client/src/utils/autocomplete/__tests__/dataTreeTypeDefCreator.test.ts(1 hunks)app/client/src/utils/editor/EditorBindingPaths.ts(2 hunks)app/client/src/utils/helpers.test.ts(1 hunks)app/client/src/utils/widgetRenderUtils.tsx(2 hunks)app/client/src/widgets/CategorySliderWidget/widget/propertyConfig/contentConfig.ts(1 hunks)app/client/src/widgets/ChartWidget/widget/propertyConfig.ts(1 hunks)app/client/src/widgets/CheckboxGroupWidget/widget/index.tsx(1 hunks)app/client/src/widgets/DropdownWidget/widget/index.tsx(2 hunks)app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx(1 hunks)app/client/src/widgets/FilepickerWidget/widget/index.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/widget/propertyConfig.ts(1 hunks)app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/common.ts(1 hunks)app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/multiSelect.ts(1 hunks)app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/radioGroup.ts(1 hunks)app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.ts(1 hunks)app/client/src/widgets/ListWidget/widget/propertyConfig.ts(1 hunks)app/client/src/widgets/ListWidgetV2/widget/propertyConfig.ts(1 hunks)app/client/src/widgets/MapChartWidget/widget/index.tsx(1 hunks)app/client/src/widgets/MapWidget/widget/index.tsx(3 hunks)app/client/src/widgets/MenuButtonWidget/widget/propertyConfig/contentConfig.ts(1 hunks)app/client/src/widgets/MultiSelectTreeWidget/widget/index.tsx(1 hunks)app/client/src/widgets/MultiSelectWidget/widget/index.tsx(1 hunks)app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(2 hunks)app/client/src/widgets/RadioGroupWidget/widget/index.tsx(1 hunks)app/client/src/widgets/SelectWidget/widget/index.tsx(2 hunks)app/client/src/widgets/SingleSelectTreeWidget/widget/index.tsx(1 hunks)app/client/src/widgets/SwitchGroupWidget/widget/index.tsx(1 hunks)app/client/src/widgets/TableWidget/widget/propertyConfig.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Basic.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/entities/DataTree/dataTreeFactory.ts
✅ Files skipped from review due to trivial changes (51)
- app/client/src/widgets/MenuButtonWidget/widget/propertyConfig/contentConfig.ts
- app/client/src/widgets/MapChartWidget/widget/index.tsx
- app/client/src/pages/Editor/Explorer/JSActions/JSActionContextMenu.tsx
- app/client/src/modules/ui-builder/ui/wds/WDSRadioGroupWidget/config/propertyPaneConfig/contentConfig.ts
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/common.ts
- app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx
- app/client/src/modules/ui-builder/ui/wds/WDSTableWidget/config/propertyPaneConfig/contentConfig.ts
- app/client/src/widgets/CheckboxGroupWidget/widget/index.tsx
- app/client/src/utils/autocomplete/AutocompleteSortRules.ts
- app/client/src/pages/Editor/Explorer/Widgets/WidgetContextMenu.tsx
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.ts
- app/client/src/constants/PropertyControlConstants.tsx
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig.ts
- app/client/src/utils/editor/EditorBindingPaths.ts
- app/client/src/utils/autocomplete/tests/AutocompleteSortRules.test.ts
- app/client/src/widgets/SingleSelectTreeWidget/widget/index.tsx
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/multiSelect.ts
- app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx
- app/client/src/ce/components/editorComponents/Debugger/ErrorLogs/getLogIconForEntity.tsx
- app/client/src/entities/Widget/utils.test.ts
- app/client/src/entities/Action/actionProperties.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMenuButtonWidget/config/propertyPaneConfig/contentConfig.ts
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Basic.ts
- app/client/src/utils/autocomplete/tests/dataTreeTypeDefCreator.test.ts
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/radioGroup.ts
- app/client/src/widgets/CategorySliderWidget/widget/propertyConfig/contentConfig.ts
- app/client/src/widgets/ChartWidget/widget/propertyConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSSwitchGroupWidget/config/propertyPaneConfig/contentConfig.ts
- app/client/src/components/editorComponents/GlobalSearch/utils.tsx
- app/client/src/ce/reducers/uiReducers/explorerReducer.ts
- app/client/src/ce/utils/FilterInternalProperties/getEntityPeekData.ts
- app/client/src/widgets/ListWidgetV2/widget/propertyConfig.ts
- app/client/src/entities/DataTree/dataTreeWidget.test.ts
- app/client/src/entities/Widget/utils.ts
- app/client/src/utils/WidgetLoadingStateUtils.test.ts
- app/client/src/modules/ui-builder/ui/wds/WDSCheckboxGroupWidget/config/propertyPaneConfig/contentConfig.ts
- app/client/src/widgets/SwitchGroupWidget/widget/index.tsx
- app/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.tsx
- app/client/src/entities/Action/actionProperties.test.ts
- app/client/src/components/editorComponents/CodeEditor/EvaluatedValuePopup.tsx
- app/client/src/utils/autocomplete/tests/TernServer.test.ts
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx
- app/client/src/widgets/ListWidget/widget/propertyConfig.ts
- app/client/src/ce/entities/DataTree/dataTreeJSAction.ts
- app/client/src/widgets/RadioGroupWidget/widget/index.tsx
- app/client/src/utils/widgetRenderUtils.tsx
- app/client/src/utils/helpers.test.ts
- app/client/src/widgets/TableWidget/widget/propertyConfig.ts
- app/client/src/entities/DataTree/dataTreeWidget.ts
- app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx
- app/client/src/components/editorComponents/CodeEditor/index.tsx
🧰 Additional context used
🧠 Learnings (1)
app/client/src/utils/autocomplete/CodemirrorTernService.ts (1)
Learnt from: ashit-rath
PR: appsmithorg/appsmith#33726
File: app/client/src/utils/autocomplete/CodemirrorTernService.ts:199-199
Timestamp: 2024-11-12T08:11:36.416Z
Learning: The `entityDef` property in `CodeMirrorTernService` class within `CodemirrorTernService.ts` is intentionally initialized as an empty object.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (28)
app/client/src/ce/entities/DataTree/isDynamicEntity.ts (1)
1-7: LGTM! Import path update aligns with PR objective.The consolidation of type imports from
ee/entities/DataTree/typeshelps reduce cyclic dependencies.app/client/src/utils/NavigationSelector/WidgetChildren.ts (1)
1-3: LGTM! Import path update is consistent.The change maintains functionality while consolidating type imports.
app/client/src/utils/NavigationSelector/JsChildren.ts (1)
1-3: LGTM! Import path update is consistent.The change aligns with the PR's goal of reducing cyclic dependencies.
app/client/src/ce/entities/DataTree/dataTreeAction.ts (1)
1-2: LGTM! Import path update is consistent.The change aligns with the PR's goal of reducing cyclic dependencies.
app/client/src/components/formControls/DynamicTextFieldControl.tsx (1)
21-21: LGTM! Import path update aligns with dependency cleanup.The change correctly updates the import path for
EvaluationSubstitutionTypeto the new centralized location.app/client/src/utils/FilterInternalProperties/__tests__/index.test.ts (1)
2-10: LGTM! Import paths correctly updated.The import paths have been properly updated to use the new centralized location for types, maintaining test functionality.
app/client/src/components/editorComponents/CodeEditor/commandsHelper.ts (1)
11-11: LGTM! Import path correctly updated.The import path for
ENTITY_TYPEhas been properly updated to the new centralized location.app/client/src/selectors/navigationSelectors.ts (2)
2-6: LGTM! Import paths correctly updated and organized.The imports have been properly updated and grouped together for better organization.
56-56: LGTM! Type export enhances type safety.The new
EntityNavigationDatatype export provides better type safety for navigation data records.app/client/src/selectors/dataTreeSelectors.ts (1)
20-21: Import path update looks good!The import path change for
ENTITY_TYPEaligns with the PR objective of reducing cyclic dependencies.app/client/src/components/editorComponents/CodeEditor/generateQuickCommands.tsx (1)
8-8: Import path update looks good!The import path change for
ENTITY_TYPEaligns with the PR objective of reducing cyclic dependencies.app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts (1)
7-7: Import path update looks good!The import path change for
EvaluationSubstitutionTypealigns with the PR objective of reducing cyclic dependencies.app/client/src/widgets/MapWidget/widget/index.tsx (1)
10-10: Import path update looks good!The import path change for
EvaluationSubstitutionTypealigns with the PR objective of reducing cyclic dependencies.app/client/src/widgets/FilepickerWidget/widget/index.tsx (3)
14-14: Import path updated to use enterprise edition types.The import path for
EvaluationSubstitutionTypehas been updated to use the enterprise edition types module.
302-304: LGTM! Default properties map implementation.The static method returns an empty object as expected for this widget.
306-311: Verify the derived properties implementation.The derived properties map defines two properties:
isValid: Checks if the widget is required and has files selectedfiles: Maps selected files with appropriate data formatPlease ensure that:
- The file data mapping handles all possible file data types
- Error handling is in place for invalid file data
app/client/src/widgets/DropdownWidget/widget/index.tsx (2)
15-15: Import path updated to use enterprise edition types.The import path for
EvaluationSubstitutionTypehas been updated to use the enterprise edition types module.
491-499: Lifecycle method implementation looks good.The
componentDidUpdatemethod correctly handles:
- Changes to
defaultOptionValue- Changes to
optionprop- Updates selected option accordingly
app/client/src/widgets/MultiSelectWidget/widget/index.tsx (1)
20-20: Import path updated to use enterprise edition types.The import path for
EvaluationSubstitutionTypehas been updated to use the enterprise edition types module.app/client/src/plugins/Linting/utils/getLintingErrors.ts (1)
45-48: Import paths updated to use enterprise edition types.The imports for
ENTITY_TYPEandAppsmithEntityhave been updated to use the enterprise edition types module.app/client/src/widgets/MultiSelectTreeWidget/widget/index.tsx (1)
9-9: LGTM! Import path update aligns with dependency cleanup.The change to import
EvaluationSubstitutionTypefromee/entities/DataTree/typeshelps reduce cyclic dependencies.app/client/src/widgets/SelectWidget/widget/index.tsx (2)
6-6: LGTM! Import path update aligns with dependency cleanup.The change to import
EvaluationSubstitutionTypefromee/entities/DataTree/typeshelps reduce cyclic dependencies.
65-65: LGTM! Widget type identifier added.Adding the static type property helps with widget identification and categorization.
app/client/src/utils/autocomplete/CodemirrorTernService.ts (2)
11-11: LGTM! Import path update aligns with dependency cleanup.The change to import
ENTITY_TYPEfromee/entities/DataTree/typeshelps reduce cyclic dependencies.
1401-1403: LGTM! Simple and focused method for updating recent entities.The method provides a clean way to update the recentEntities list used for autocomplete sorting.
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (3)
7-7: LGTM! Import path update aligns with dependency reduction.The change in import path for
EvaluationSubstitutionTypehelps reduce cyclic dependencies.
889-954: LGTM! Well-implemented event handlers.The event handlers (onOptionChange, onFilterChange, onDropdownOpen, onDropdownClose) are properly implemented with:
- Consistent error handling
- Proper type safety
- Clear execution patterns
957-994: LGTM! Clear and well-typed interfaces.The interfaces (OptionValue, DropdownOption, MultiSelectWidgetProps) are properly defined with clear types and good documentation.
Description
Remove specific cycle dependencies created because a file was both importing and exporting items it does not modify.
By reducing cycle dependencies here, I am buying credits that allow adding cycle dependencies in another PR but very hard to avoid
This PR reduces the deps by 4
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/6214
PR which adds the cycle deps: #39100
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13324385763
Commit: 34a0f45
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 14 Feb 2025 07:54:00 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Improvements