chore: Remove unnecessary properties from being stored in Anvil DSLs#35088
chore: Remove unnecessary properties from being stored in Anvil DSLs#35088
Conversation
WalkthroughThe recent changes enhance the efficiency and clarity of the Anvil DSL by decoupling its widget addition flow from Fixed and Auto Layout systems. A new static property effectively manages default widget properties, while various utility functions have been refactored for better maintainability and performance. These updates streamline widget integration within the application, leading to improved organization and user experience. Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
…o chore/cleanup-anvil-dsl
riodeuno
changed the title
github-actions
bot
added
Anvil Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts (1)
29-29: Fix typo in comment.There is a typo in the comment: "hydation" should be "hydration".
- // For WDS_INLINE_BUTTONS_WIDGET, we want to hydation only to work when we add more items to the inline button group. + // For WDS_INLINE_BUTTONS_WIDGET, we want hydration only to work when we add more items to the inline button group.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (17)
- app/client/src/WidgetProvider/factory/index.tsx (2 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/anvilDraggingSagas.test.ts (2 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/helpers.ts (1 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts (3 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts (1 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts (1 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/index.ts (2 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/update/mainCanvasLayoutUtils.ts (2 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/update/sectionUtils.ts (3 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (4 hunks)
- app/client/src/layoutSystems/anvil/utils/sectionOperationUtils.ts (2 hunks)
- app/client/src/layoutSystems/anvil/utils/widgetAdditionUtils.ts (2 hunks)
- app/client/src/sagas/WidgetAdditionSagas.ts (5 hunks)
- app/client/src/widgets/anvil/ZoneWidget/widget/config/defaultConfig.ts (1 hunks)
- app/client/src/widgets/wds/WDSInputWidget/config/defaultsConfig.ts (1 hunks)
- app/client/src/widgets/wds/WDSModalWidget/config/defaultsConfig.ts (1 hunks)
- app/client/src/widgets/wds/WDSTableWidget/config/defaultsConfig.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/wds/WDSModalWidget/config/defaultsConfig.ts
Additional context used
Biome
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
[error] 140-140: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (45)
app/client/src/widgets/wds/WDSInputWidget/config/defaultsConfig.ts (1)
9-9: Verify the implications of changing the version from2to1.Changing the version property from
2to1might have broader implications on how the widget behaves or is rendered. Ensure that this change is backward compatible and does not introduce any regressions.app/client/src/layoutSystems/anvil/integrations/sagas/index.ts (2)
17-17: LGTM! Ensure the new saga does not introduce side effects.The code changes are approved.
However, ensure that the new saga does not introduce any unintended side effects or conflicts with existing sagas.
8-8: Ensure proper implementation and integration ofanvilWidgetAdditionSagas.The addition of
anvilWidgetAdditionSagasmodifies the control flow and allows it to run concurrently with existing sagas. Verify that this new saga is correctly implemented and integrated.Verification successful
Let's correct the previous command and gather the necessary information to verify the implementation and integration of
anvilWidgetAdditionSagas.
The implementation and integration of
anvilWidgetAdditionSagasare correct.The
anvilWidgetAdditionSagasis properly defined and integrated into the saga system. It is imported and forked within theanvilSagasfunction, ensuring it runs concurrently with other sagas.
- File:
app/client/src/layoutSystems/anvil/integrations/sagas/index.ts
- Line: 8
- Details: The
anvilWidgetAdditionSagasis imported and forked within theanvilSagasgenerator function.- File:
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
- Details: The
anvilWidgetAdditionSagasfunction is defined to handle specific action types usingtakeLatest.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and integration of `anvilWidgetAdditionSagas`. # Test: Search for the implementation details of `anvilWidgetAdditionSagas`. Expect: Proper implementation and integration. rg --type javascript -A 10 $'anvilWidgetAdditionSagas'Length of output: 88
Script:
#!/bin/bash # Description: Verify the implementation and integration of `anvilWidgetAdditionSagas`. # Search for the implementation details of `anvilWidgetAdditionSagas`. rg 'anvilWidgetAdditionSagas' -A 10 # Search for the import statements of `anvilWidgetAdditionSagas` to understand its integration. rg 'import anvilWidgetAdditionSagas' -A 5Length of output: 9138
app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/helpers.ts (1)
1-32: Verify the performance and correctness ofgetMainCanvasLastRowHighlight.The function
getMainCanvasLastRowHighlightretrieves highlighting information for the last row in the main canvas layout. Ensure that the function performs efficiently and returns correct data.app/client/src/widgets/wds/WDSTableWidget/config/defaultsConfig.ts (1)
39-39: Ensure compatibility with version change.The
versionfield in thedefaultsConfigobject has been changed from2to1. This might affect how the application interprets the configuration settings. Ensure that this change does not introduce any incompatibilities or regressions.app/client/src/widgets/anvil/ZoneWidget/widget/config/defaultConfig.ts (2)
46-60: Good job on handling modal widget hierarchy!The new logic block correctly processes the widget hierarchy to determine if a widget is contained within a modal. This ensures that modal widgets do not display an elevated background by default, improving visual consistency.
33-34: Clarify the purpose of theparentparameter.The comment questions the necessity of the
parentparameter. Ensure that this parameter is indeed required or remove it if it is redundant.Verification successful
Let's correct the file type and search for the usage of the
parentparameter in TypeScript files to determine its necessity.
The
parentparameter is necessary.The
parentparameter is used extensively across multiple files in the codebase, indicating its importance in the widget system. Removing it could lead to unintended consequences.
app/client/src/widgets/FormWidget/widget/index.tsxapp/client/src/widgets/wds/WDSModalWidget/config/defaultsConfig.tsapp/client/src/widgets/anvil/SectionWidget/widget/config/defaultConfig.tsapp/client/src/widgets/anvil/ZoneWidget/widget/config/defaultConfig.tsapp/client/src/widgets/StatboxWidget/widget/index.tsxapp/client/src/widgets/ListWidgetV2/widget/defaultProps.tsapp/client/src/widgets/ContainerWidget/widget/index.tsxapp/client/src/utils/WidgetPropsUtils.tsxapp/client/src/sagas/WidgetAdditionSagas.tsapp/client/src/sagas/WidgetBlueprintSagas.tsapp/client/src/sagas/CanvasSagas/DraggingCanvasSagas.tsapp/client/src/layoutSystems/autolayout/utils/heightUpdateUtils.tsapp/client/src/layoutSystems/anvil/utils/paste/mainCanvasPasteUtils.tsapp/client/src/layoutSystems/anvil/utils/paste/zonePasteUtils.tsapp/client/src/layoutSystems/anvil/utils/paste/sectionPasteUtils.tsapp/client/src/layoutSystems/anvil/utils/anvilChecksUtils.tsapp/client/src/layoutSystems/anvil/utils/paste/destinationUtils/index.tsapp/client/src/layoutSystems/anvil/utils/paste/utils.tsapp/client/src/layoutSystems/anvil/integrations/sagas/pasteSagas/index.tsapp/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.tsapp/client/src/WidgetProvider/factory/index.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `parent` parameter in the codebase to ensure it is necessary. # Test: Search for the usage of the `parent` parameter. Expect: Relevant usage or redundancy. rg --type python -A 5 $'parent: FlattenedWidgetProps'Length of output: 85
Script:
#!/bin/bash # Description: Verify the usage of the `parent` parameter in the codebase to ensure it is necessary. # Test: Search for the usage of the `parent` parameter in TypeScript files. rg --type ts -A 5 'parent: FlattenedWidgetProps'Length of output: 14937
app/client/src/layoutSystems/anvil/utils/widgetAdditionUtils.ts (1)
1-1: Ensure the logic for creating widget payloads is refactored or relocated.The
getCreateWidgetPayloadfunction has been removed. Ensure that the logic for creating widget payloads has been refactored or relocated appropriately to avoid any issues with widget instantiation.app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts (5)
Line range hint
1-17:
Good job on removing unused imports!This helps in keeping the code clean and maintainable.
Line range hint
26-45:
Well-structured function with proper error handling!The
moveWidgetsSagafunction is correctly implemented and handles errors appropriately.
Line range hint
47-68:
Good use of utility functions!The
handleWidgetMovementfunction is correctly implemented and uses appropriate utility functions.
Line range hint
70-98:
Great job on handling redundant zones!The
handleDeleteRedundantZonesfunction is correctly implemented and uses appropriate utility functions.
Line range hint
100-104:
Nice simplification of the default export function!This helps in keeping the code clean and maintainable.
app/client/src/layoutSystems/anvil/utils/layouts/update/mainCanvasLayoutUtils.ts (4)
Line range hint
1-16:
Necessary import added!The import statement for
addNewAnvilWidgetToDSLis necessary for the updated functionality.
29-37: Simplified function and improved performance!The changes to
addDetachedWidgetToMainCanvassimplify the function and improve performance by removing an intermediate function call.
Line range hint
39-92:
Well-structured function with appropriate utility functions!The
addWidgetsToMainCanvasLayoutfunction is correctly implemented and uses appropriate utility functions.
Line range hint
153-170:
Good job on moving widgets to the main canvas!The
moveWidgetsToMainCanvasfunction is correctly implemented and uses appropriate utility functions.app/client/src/layoutSystems/anvil/utils/sectionOperationUtils.ts (2)
Line range hint
1-17:
Necessary import added!The import statement for
getUpdatedListOfWidgetsAfterAddingNewWidgetis necessary for the updated functionality.
Line range hint
161-176:
Improved widget management!The changes to
addNewZonesToSectionindicate a shift in how new widgets are integrated into the existing widget structure, potentially improving the overall responsiveness or correctness of widget management within the application.app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (5)
Line range hint
26-44:
Good use of the new functionaddNewAnvilWidgetToDSL.The changes improve readability and maintainability by using an object format for parameters.
However, ensure that the new function is correctly integrated and tested.
Verification successful
Good integration of the function
addNewAnvilWidgetToDSL.The function is consistently used across multiple modules, improving readability and maintainability. Ensure the function implementation in
helpers.tsis correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `addNewAnvilWidgetToDSL`. # Test: Search for the function definition and its usage. Expect: Correct integration and usage. rg --type ts -A 5 $'addNewAnvilWidgetToDSL'Length of output: 7550
179-183: Good use of the new functionaddNewAnvilWidgetToDSL.The changes improve readability and maintainability by using an object format for parameters.
However, ensure that the new function is correctly integrated and tested.
Verification successful
The integration of
addNewAnvilWidgetToDSLappears to be consistent and correct across the codebase.The function is imported and utilized in multiple files, maintaining a standardized approach. This enhances readability and maintainability.
- Files with usage:
sectionUtils.tsmainCanvasLayoutUtils.tsindex.tszoneUtils.tsEnsure thorough testing to validate the functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `addNewAnvilWidgetToDSL`. # Test: Search for the function definition and its usage. Expect: Correct integration and usage. rg --type ts -A 5 $'addNewAnvilWidgetToDSL'Length of output: 7550
Line range hint
64-65:
Verify the integration ofupdateDraggedWidgets.Ensure that the changes in
updateDraggedWidgetsare correctly reflected inaddWidgetsToZone.Verification successful
Let's verify the integration of
updateDraggedWidgetswithinzoneUtils.tsto ensure that the changes are correctly reflected.
Verify the integration of
updateDraggedWidgets.The integration of the
updateDraggedWidgetsfunction withinzoneUtils.tsappears to be correct. The function is used to update theupdatedWidgetsvariable, which is then used to updatezoneProps.children. This indicates that the changes inupdateDraggedWidgetsare correctly reflected in its usage.
- File:
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts- Lines: 10-15
Review Outcome: No issues found with the integration.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `updateDraggedWidgets`. # Test: Search for the function definition and its usage. Expect: Correct integration and usage. rg --type ts -A 5 $'updateDraggedWidgets'Length of output: 1248
Script:
#!/bin/bash # Description: Extract full context of the `updateDraggedWidgets` call within `zoneUtils.ts`. # Test: Display lines around the call to `updateDraggedWidgets` for detailed review. rg --type ts -A 10 -B 10 'updateDraggedWidgets' app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.tsLength of output: 1461
Line range hint
232-233:
Verify the integration ofmoveWidgetsToNewLayout.Ensure that the changes in
moveWidgetsToNewLayoutare correctly reflected inmoveWidgetsToZone.Verification successful
Ensure the integration of
moveWidgetsToNewLayoutwithinmoveWidgetsToZoneThe integration appears to be correctly implemented. The function
moveWidgetsToZonecallsmoveWidgetsToNewLayoutwhen a large widget is present, ensuring the logic flows as intended.
- File:
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts
- Function Definition:
export function* moveWidgetsToZone( allWidgets: CanvasWidgetsReduxState, movedWidgets: string[], highlight: AnvilHighlightInfo, ) { const widgets: CanvasWidgetsReduxState = { ...allWidgets }; const draggedWidgets: WidgetLayoutProps[] = transformMovedWidgets( widgets, movedWidgets, highlight, ); const isLargeWidgetPresent = draggedWidgets.some((each) => isLargeWidget(each.widgetType), ); if (isLargeWidgetPresent) { const canvasWidgets: CanvasWidgetsReduxState = yield call( moveWidgetsToNewLayout, widgets, movedWidgets, highlight, ); return canvasWidgets; } else { const updatedWidgets = moveWidgets(allWidgets, movedWidgets, highlight); return updatedWidgets; } }This confirms that
moveWidgetsToZonecorrectly integrates the updatedmoveWidgetsToNewLayout.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `moveWidgetsToNewLayout`. # Test: Search for the function definition and its usage. Expect: Correct integration and usage. rg --type ts -A 5 $'moveWidgetsToNewLayout'Length of output: 1184
Script:
#!/bin/bash # Description: Verify the integration of `moveWidgetsToNewLayout` in `moveWidgetsToZone`. # Locate the definition of `moveWidgetsToZone` and its usage. rg --type ts -A 10 $'function moveWidgetsToZone' # Locate calls to `moveWidgetsToNewLayout` within `moveWidgetsToZone`. rg --type ts -A 5 $'moveWidgetsToZone' | rg --type ts -A 5 $'moveWidgetsToNewLayout'Length of output: 130
Script:
#!/bin/bash # Description: Verify the integration of `moveWidgetsToNewLayout` in `moveWidgetsToZone`. # Locate the definition of `moveWidgetsToZone`. rg --type ts -A 10 'function moveWidgetsToZone' # Locate calls to `moveWidgetsToNewLayout` within `moveWidgetsToZone`. rg --type ts -A 10 'moveWidgetsToZone' | rg --type ts -A 10 'moveWidgetsToNewLayout'Length of output: 132
Script:
#!/bin/bash # Description: Verify the integration of `moveWidgetsToNewLayout` in `moveWidgetsToZone`. # Locate the occurrences of `moveWidgetsToZone` and `moveWidgetsToNewLayout` with broader context. rg --type ts -A 20 'moveWidgetsToZone' rg --type ts -A 20 'moveWidgetsToNewLayout'Length of output: 10841
Line range hint
194-195:
Verify the integration ofaddWidgetsToZone.Ensure that the changes in
addWidgetsToZoneare correctly reflected inmoveWidgetsToNewLayout.app/client/src/layoutSystems/anvil/utils/layouts/update/sectionUtils.ts (4)
Line range hint
17-35:
Good use of the new functionaddNewAnvilWidgetToDSL.The changes improve readability and maintainability by using an object format for parameters.
However, ensure that the new function is correctly integrated and tested.
Verification successful
Good use of the new function
addNewAnvilWidgetToDSL.The changes improve readability and maintainability by using an object format for parameters. The function appears to be correctly integrated across various parts of the codebase.
sectionUtils.ts: Correctly usesaddNewAnvilWidgetToDSL.zoneUtils.ts: Correctly usesaddNewAnvilWidgetToDSL.mainCanvasLayoutUtils.ts: Correctly usesaddNewAnvilWidgetToDSL.helpers.ts: Contains the function definition.The integration seems to be thorough and consistent.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `addNewAnvilWidgetToDSL`. # Test: Search for the function definition and its usage. Expect: Correct integration and usage. rg --type ts -A 5 $'addNewAnvilWidgetToDSL'Length of output: 7550
Line range hint
263-264:
Verify the integration ofaddWidgetsToSection.Ensure that the changes in
addWidgetsToSectionare correctly reflected inmoveWidgetsToSection.Verification successful
Let's verify the integration of the
addWidgetsToSectionfunction within themoveWidgetsToSectionfunction. We need to ensure that the changes inaddWidgetsToSectionare correctly reflected in its usage.
Let's broaden our search to ensure we capture the definition and usage of
moveWidgetsToSectionaccurately.
Verify the integration of
addWidgetsToSectionwithinmoveWidgetsToSection.The
moveWidgetsToSectionfunction correctly integrates theaddWidgetsToSectionfunction. The changes inaddWidgetsToSectionare reflected in its usage withinmoveWidgetsToSection, ensuring proper functionality.
Location of
moveWidgetsToSectiondefinition:
app/client/src/layoutSystems/anvil/utils/layouts/update/sectionUtils.tsLocation of
moveWidgetsToSectionusage:
app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `addWidgetsToSection`. # Test: Search for the function definition and its usage. Expect: Correct integration and usage. rg --type ts -A 5 $'addWidgetsToSection'Length of output: 3338
Script:
#!/bin/bash # Description: Verify the integration of `addWidgetsToSection` within `moveWidgetsToSection`. # Search for the definition and usage of `moveWidgetsToSection`. rg --type ts -A 10 $'function moveWidgetsToSection'Length of output: 52
Script:
#!/bin/bash # Description: Verify the integration of `addWidgetsToSection` within `moveWidgetsToSection`. # Search for the definition and usage of `moveWidgetsToSection`. rg --type ts -A 20 'moveWidgetsToSection'Length of output: 6621
Line range hint
145-146:
Verify the integration ofaddZoneToSection.Ensure that the changes in
addZoneToSectionare correctly reflected inaddWidgetsToSection.
96-100: Good use of the new functionaddNewAnvilWidgetToDSL.The changes improve readability and maintainability by using an object format for parameters.
However, ensure that the new function is correctly integrated and tested.
app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/anvilDraggingSagas.test.ts (1)
2-2: Verify the new import ofaddWidgetsSaga.Ensure that the new import is correctly integrated and tested.
Also applies to: 36-36
Verification successful
The new import of
addWidgetsSagais correctly integrated and tested.
- The
addWidgetsSagafunction is imported from../anvilWidgetAdditionSagas.- It is used appropriately within the test file
anvilDraggingSagas.test.ts.- The function is defined and utilized correctly within
anvilWidgetAdditionSagas/index.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new import of `addWidgetsSaga`. # Test: Search for the function definition and its usage. Expect: Correct integration and usage. rg --type ts -A 5 $'addWidgetsSaga'Length of output: 3965
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts (3)
163-208: LGTM!The
addWidgetsSagafunction is well-structured and includes necessary error handling.
210-251: LGTM!The
addWidgetToGenericLayoutfunction is well-structured and includes necessary operations.
253-261: LGTM!The
anvilWidgetAdditionSagasfunction is well-structured and correctly sets up the sagas.app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts (5)
69-83: LGTM!The
getUniqueWidgetNamefunction is well-structured and includes necessary logic to ensure uniqueness.
93-114: LGTM!The
runBlueprintOperationsOnWidgetsfunction is well-structured and includes necessary logic to handle blueprint operations.Tools
Biome
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
125-139: LGTM!The
addChildReferenceToParentfunction is well-structured and includes necessary logic to update the parent's children list.
149-201: LGTM!The
updateWidgetListWithNewWidgetfunction is well-structured and includes necessary logic to handle widget addition.
212-248: LGTM!The
addNewAnvilWidgetToDSLfunction is well-structured and includes necessary logic to handle widget addition.app/client/src/sagas/WidgetAdditionSagas.ts (5)
Line range hint
1-1:
Verify the impact of the removal ofgetWidgetSessionValues.The removal of
getWidgetSessionValuesmay impact the functionality ofgetChildWidgetProps. Ensure that the necessary properties are still being hydrated correctly.
Line range hint
1-1:
LGTM!The
generateChildWidgetsfunction is well-structured and includes necessary logic to handle child widget generation.
Line range hint
1-1:
LGTM!The
getUpdateDslAfterCreatingChildfunction is well-structured and includes necessary logic to handle DSL updates.
Line range hint
1-1:
LGTM!The
addChildSagafunction is well-structured and includes necessary error handling.
Line range hint
1-1:
LGTM!The
addNewTabChildSagafunction is well-structured and includes necessary logic to handle tab child addition.app/client/src/WidgetProvider/factory/index.tsx (3)
58-60: Good addition:widgetDefaultPropertiesMapThe introduction of
widgetDefaultPropertiesMapis a smart move to manage default widget properties efficiently. By storing default properties for each widget type, it helps in reducing the number of properties passed during widget creation, thereby optimizing the DSL and preventing unnecessary clutter.
138-145: Clear and helpful commentsThe comments explaining the purpose of
widgetDefaultPropertiesMapare clear and provide valuable context. They effectively communicate the rationale behind using the map to optimize the widget addition process and reduce unnecessary properties in the DSL.
146-149: Good use of immutability withObject.freezeThe use of
Object.freezeto set default properties inwidgetDefaultPropertiesMapis commendable. It ensures that the default properties remain immutable, preventing any unintended modifications and maintaining the integrity of the widget configurations.
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
Show resolved
Hide resolved
|
@KelvinOm @znamenskii-ilia I'm adding some unit tests to this PR. It should be ready sometime today. I've requested for review a little earlier to provide enough time to review the main code. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10082900888. |
|
Deploy-Preview-URL: https://ce-35088.dp.appsmith.com |
app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/helpers.ts
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts
Outdated
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts
Show resolved
Hide resolved
KelvinOm
left a comment
KelvinOm
left a comment
There was a problem hiding this comment.
Our DSL has become much smaller and cleaner 👍
I left a few comments. I also analysed the new DSL structure and found few attributes that should not be there. Can we fix this now?
MainContainer
- rightColumn
- topRow
- bottomRow
- snapRows
- minHeight
- leftColumn
- parentColumnSpace
Section
- boxShadow
- rows
- columns
- detachFromLayout
- layoutStyle
Zone
- flexGrow » should be renamed to spaceDistributed
- detachFromLayout
- layoutStyle
- columns
- rows
- flexVerticalAlignment
For all Input like widgets
- showStepArrows
- labelPosition
For all widgets
- responsiveBehavior
- layoutStyle
app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/helpers.ts
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
Show resolved
Hide resolved
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts
Show resolved
Hide resolved
|
@KelvinOm Thanks for reviewing the list of properties. MainContainerI can't remove these properties in this PR. This needs a backend change where the backend now needs to be aware of which layout system we're serving and use a different template. Today, when creating a page the backend uses an existing template which was defined for Fixed mode. Section and ZonesI've removed most of what you've suggested. I have no idea where All other widgetsThese properties are coming from the default configuration of the widgets, we'll have to create a new PR after auditing and removing these properties from the widget configurations. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/helpers.ts (1 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.test.ts (1 hunks)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts (1 hunks)
- app/client/src/layoutSystems/anvil/layoutComponents/presets/sectionPreset.tsx (1 hunks)
- app/client/src/layoutSystems/anvil/layoutComponents/presets/zonePreset.tsx (1 hunks)
- app/client/src/widgets/anvil/SectionWidget/widget/config/defaultConfig.ts (1 hunks)
- app/client/src/widgets/anvil/ZoneWidget/widget/config/defaultConfig.ts (3 hunks)
Files skipped from review due to trivial changes (2)
- app/client/src/layoutSystems/anvil/layoutComponents/presets/sectionPreset.tsx
- app/client/src/layoutSystems/anvil/layoutComponents/presets/zonePreset.tsx
Files skipped from review as they are similar to previous changes (3)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/helpers.ts
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.ts
- app/client/src/widgets/anvil/ZoneWidget/widget/config/defaultConfig.ts
Additional comments not posted (5)
app/client/src/widgets/anvil/SectionWidget/widget/config/defaultConfig.ts (1)
14-14: Verify the impact of property removals.The properties
boxShadowanddetachFromLayouthave been removed from thedefaultConfig. Ensure that these properties are not required elsewhere in the codebase and that their removal does not affect the widget's functionality.app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/helpers.test.ts (4)
16-35: Test case foraddChildReferenceToParentlooks good!The test case correctly verifies that a new child reference is added to the parent widget.
57-74: Test case forgetUniqueWidgetNamelooks good!The test case correctly verifies that a unique widget name is generated within the data tree.
77-116: Test case forrunBlueprintOperationsOnWidgetslooks good!The test case correctly verifies that blueprint operations are executed on widgets as expected.
119-195: Test case forupdateWidgetListWithNewWidgetlooks good!The test case correctly verifies that the widget list is updated with a new widget as expected.
|
/build-deploy-preview skip-tests=true |
riodeuno
added
ok-to-test
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10099578462. |
|
Deploy-Preview-URL: https://ce-35088.dp.appsmith.com |
@riodeuno Could you create tasks in our backlog to ensure that we don't forget anything here? |
3 participants
Description
widgetAdditionSagahave been removed from the flow when adding new Anvil widgets to Canvas##Tasks
Fixes #34896
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10099580771
Commit: 50d9f03
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 25 Jul 2024 19:31:29 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation